You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/10/19 06:32:23 UTC
[GitHub] [calcite] chunweilei opened a new pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
chunweilei opened a new pull request #2224:
URL: https://github.com/apache/calcite/pull/2224
… expression with wrong row type
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r507516757
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType other,
+ RelDataType another) {
+ if (other == another) {
+ return "";
+ }
+
+ if (other.getFieldCount() != another.getFieldCount()) {
+ return "Type mismatch: the field size is not equal,"
+ + " one is " + other.getFieldCount()
+ + ", another one is " + another.getFieldCount();
Review comment:
Please, remove that.
The field count says nothing, and it would be a nightmare to debug errors saying `"one is 5, another one is 6"`
Please incorporate that into "extra field" / "field removed" message.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r508171035
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
Review comment:
Sometimes users want to know both what the source type or the target type is and their difference.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r507637181
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType other,
+ RelDataType another) {
+ if (other == another) {
+ return "";
+ }
+
+ if (other.getFieldCount() != another.getFieldCount()) {
+ return "Type mismatch: the field size is not equal,"
+ + " one is " + other.getFieldCount()
+ + ", another one is " + another.getFieldCount();
+ }
+
+ final StringBuilder stringBuilder = new StringBuilder("Type mismatch: \n");
+ final List<RelDataTypeField> f1 = other.getFieldList();
+ final List<RelDataTypeField> f2 = another.getFieldList();
+ for (Pair<RelDataTypeField, RelDataTypeField> pair : Pair.zip(f1, f2)) {
+ final RelDataType type1 = pair.left.getType();
+ final RelDataType type2 = pair.right.getType();
+ // If one of the types is ANY comparison should succeed
+ if (type1.getSqlTypeName() == SqlTypeName.ANY
+ || type2.getSqlTypeName() == SqlTypeName.ANY) {
Review comment:
Do you mean this method is the same as `#areRowTypesEqual`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r507830151
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType other,
+ RelDataType another) {
+ if (other == another) {
+ return "";
+ }
+
+ if (other.getFieldCount() != another.getFieldCount()) {
+ return "Type mismatch: the field size is not equal,"
+ + " one is " + other.getFieldCount()
+ + ", another one is " + another.getFieldCount();
+ }
+
+ final StringBuilder stringBuilder = new StringBuilder("Type mismatch: \n");
+ final List<RelDataTypeField> f1 = other.getFieldList();
+ final List<RelDataTypeField> f2 = another.getFieldList();
+ for (Pair<RelDataTypeField, RelDataTypeField> pair : Pair.zip(f1, f2)) {
+ final RelDataType type1 = pair.left.getType();
+ final RelDataType type2 = pair.right.getType();
+ // If one of the types is ANY comparison should succeed
+ if (type1.getSqlTypeName() == SqlTypeName.ANY
+ || type2.getSqlTypeName() == SqlTypeName.ANY) {
Review comment:
I mean the comparison rules should better be reused rather than duplicated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r510702256
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,57 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType source,
+ RelDataType target) {
+ if (source == target) {
+ return "";
+ }
+
+ final int sourceFieldCount = source.getFieldCount();
+ final int targetFieldCount = target.getFieldCount();
+ if (sourceFieldCount != targetFieldCount) {
+ return "Type mismatch: the field size of source and target are not equal, "
+ + "the source type has " + sourceFieldCount + " fields, "
+ + "while the target type has " + targetFieldCount + " fields";
Review comment:
@vlsi If the size is not equal, the user should fix it first. Then the user would know the difference if the types are not equal.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r507676458
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType other,
+ RelDataType another) {
+ if (other == another) {
+ return "";
+ }
+
+ if (other.getFieldCount() != another.getFieldCount()) {
+ return "Type mismatch: the field size is not equal,"
+ + " one is " + other.getFieldCount()
+ + ", another one is " + another.getFieldCount();
+ }
+
+ final StringBuilder stringBuilder = new StringBuilder("Type mismatch: \n");
+ final List<RelDataTypeField> f1 = other.getFieldList();
+ final List<RelDataTypeField> f2 = another.getFieldList();
+ for (Pair<RelDataTypeField, RelDataTypeField> pair : Pair.zip(f1, f2)) {
+ final RelDataType type1 = pair.left.getType();
+ final RelDataType type2 = pair.right.getType();
+ // If one of the types is ANY comparison should succeed
+ if (type1.getSqlTypeName() == SqlTypeName.ANY
+ || type2.getSqlTypeName() == SqlTypeName.ANY) {
Review comment:
yes, the method is the same.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei merged pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
chunweilei merged pull request #2224:
URL: https://github.com/apache/calcite/pull/2224
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r507518209
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType other,
+ RelDataType another) {
+ if (other == another) {
+ return "";
+ }
+
+ if (other.getFieldCount() != another.getFieldCount()) {
+ return "Type mismatch: the field size is not equal,"
+ + " one is " + other.getFieldCount()
+ + ", another one is " + another.getFieldCount();
+ }
Review comment:
It is better if we can make a difference which is the `source` type and which is the `target` type.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r509007007
##########
File path: core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTest.java
##########
@@ -296,6 +300,32 @@
"PhysSingleRel:RelSubset#0.PHYS.[]")));
}
+ @Test void testTypeMismatch() {
+ VolcanoPlanner planner = new VolcanoPlanner();
+ planner.addRelTraitDef(ConventionTraitDef.INSTANCE);
+ planner.addRule(MockPhysLeafRule.INSTANCE);
+
+ RelOptCluster cluster = newCluster(planner);
+ NoneLeafRel leafRel =
+ new NoneLeafRel(
+ cluster,
+ "a");
+ RelNode convertedRel =
+ planner.changeTraits(
+ leafRel,
+ cluster.traitSetOf(PHYS_CALLING_CONVENTION));
+ planner.setRoot(convertedRel);
+ try {
+ planner.chooseDelegate().findBestExp();
+ } catch (RuntimeException e) {
Review comment:
Done. Thank you for your advice.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r508346994
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType source,
+ RelDataType target) {
+ if (source == target) {
+ return "";
+ }
+
+ if (source.getFieldCount() > target.getFieldCount()) {
+ return "Type mismatch: the target type has less fields than the source type";
+ } else if (source.getFieldCount() < target.getFieldCount()) {
+ return "Type mismatch: the target type has more fields than the source type";
+ }
+
+ final StringBuilder stringBuilder = new StringBuilder("Type mismatch: \n");
+ final List<RelDataTypeField> f1 = source.getFieldList();
+ final List<RelDataTypeField> f2 = target.getFieldList();
+ for (Pair<RelDataTypeField, RelDataTypeField> pair : Pair.zip(f1, f2)) {
+ final RelDataType type1 = pair.left.getType();
+ final RelDataType type2 = pair.right.getType();
+ // If one of the types is ANY comparison should succeed
+ if (type1.getSqlTypeName() == SqlTypeName.ANY
+ || type2.getSqlTypeName() == SqlTypeName.ANY) {
+ continue;
+ }
+ if (!type1.equals(type2)) {
+ stringBuilder.append(pair.left.getName());
+ stringBuilder.append(": ");
+ stringBuilder.append(type1.getFullTypeString());
+ stringBuilder.append(" -> ");
+ stringBuilder.append(type2.getFullTypeString());
+ stringBuilder.append("\n");
+ }
+ }
+ return stringBuilder.toString();
Review comment:
If stringBuilder is not empty, we should add the prefix "Type mismatch"?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r508348124
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType source,
+ RelDataType target) {
+ if (source == target) {
+ return "";
+ }
+
+ if (source.getFieldCount() > target.getFieldCount()) {
+ return "Type mismatch: the target type has less fields than the source type";
+ } else if (source.getFieldCount() < target.getFieldCount()) {
+ return "Type mismatch: the target type has more fields than the source type";
+ }
+
+ final StringBuilder stringBuilder = new StringBuilder("Type mismatch: \n");
Review comment:
If the fields are identical, we should not have the prefix "Type mismatch"?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r508171890
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType other,
+ RelDataType another) {
+ if (other == another) {
+ return "";
+ }
+
+ if (other.getFieldCount() != another.getFieldCount()) {
+ return "Type mismatch: the field size is not equal,"
+ + " one is " + other.getFieldCount()
+ + ", another one is " + another.getFieldCount();
+ }
+
+ final StringBuilder stringBuilder = new StringBuilder("Type mismatch: \n");
+ final List<RelDataTypeField> f1 = other.getFieldList();
+ final List<RelDataTypeField> f2 = another.getFieldList();
+ for (Pair<RelDataTypeField, RelDataTypeField> pair : Pair.zip(f1, f2)) {
+ final RelDataType type1 = pair.left.getType();
+ final RelDataType type2 = pair.right.getType();
+ // If one of the types is ANY comparison should succeed
+ if (type1.getSqlTypeName() == SqlTypeName.ANY
+ || type2.getSqlTypeName() == SqlTypeName.ANY) {
Review comment:
I could not figure out a way to reuse those codes elegantly. Feel free to improve it if you have one~~
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r507519887
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType other,
+ RelDataType another) {
+ if (other == another) {
+ return "";
+ }
+
+ if (other.getFieldCount() != another.getFieldCount()) {
+ return "Type mismatch: the field size is not equal,"
+ + " one is " + other.getFieldCount()
+ + ", another one is " + another.getFieldCount();
+ }
+
+ final StringBuilder stringBuilder = new StringBuilder("Type mismatch: \n");
+ final List<RelDataTypeField> f1 = other.getFieldList();
+ final List<RelDataTypeField> f2 = another.getFieldList();
+ for (Pair<RelDataTypeField, RelDataTypeField> pair : Pair.zip(f1, f2)) {
+ final RelDataType type1 = pair.left.getType();
+ final RelDataType type2 = pair.right.getType();
+ // If one of the types is ANY comparison should succeed
+ if (type1.getSqlTypeName() == SqlTypeName.ANY
+ || type2.getSqlTypeName() == SqlTypeName.ANY) {
Review comment:
Do you think the method could take a lambda for the equivalence rather than baking the logic here?
I think we should use reuse the comparison rather than duplicate logic.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r510502991
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,57 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType source,
+ RelDataType target) {
+ if (source == target) {
+ return "";
+ }
+
+ final int sourceFieldCount = source.getFieldCount();
+ final int targetFieldCount = target.getFieldCount();
+ if (sourceFieldCount != targetFieldCount) {
+ return "Type mismatch: the field size of source and target are not equal, "
+ + "the source type has " + sourceFieldCount + " fields, "
+ + "while the target type has " + targetFieldCount + " fields";
Review comment:
The message does not help developers.
Here's a long-standing issue for JUnit4 which was finally fixed in 4.13
https://github.com/junit-team/junit4/blob/HEAD/doc/ReleaseNotes4.13.md#pull-request-1300-show-contents-of-actual-array-when-array-lengths-differ
> Previously, when comparing two arrays which differ in length, assertArrayEquals() would only report that they differ in length. Now, it does the usual array comparison even when arrays differ in length, producing a failure message which combines the difference in length and the first difference in content. Where the content is another array, it is described by its type and length
That is why "source has 5 fields, target has 4 fields" is not really much better than "something is different", and it indeed is way worse than the previous error which produced full type dump. At least, there was a possibility to manually inspect and compare types, and now it is lost.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r507715819
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
Review comment:
Yes. Some classes use the method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r507828648
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
Review comment:
It looks like the error message would duplicate full types multiple times: once from `type1.getFullTypeString()` and the second time would be from `getFullTypeDifferenceString`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r507829475
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType source,
+ RelDataType target) {
+ if (source == target) {
+ return "";
+ }
+
+ if (source.getFieldCount() > target.getFieldCount()) {
+ return "Type mismatch: the target type has less fields than the source type";
Review comment:
I'm afraid `Type mismatch: the target type has less fields than the source type` provides very little information to the developer on what is wrong.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r508963492
##########
File path: core/src/test/java/org/apache/calcite/plan/RelOptUtilTest.java
##########
@@ -130,6 +130,43 @@
Util.toLinux(RelOptUtil.dumpType(t2) + "\n"));
}
+ @Test void testTypeDifference() {
+ final RelDataTypeFactory typeFactory =
+ new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+ final RelDataType t1 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 5, 2)
+ .add("f1", SqlTypeName.VARCHAR, 10)
+ .build();
+
+ RelDataType t2 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 5, 2)
+ .add("f1", SqlTypeName.VARCHAR, 5)
+ .build();
+
+ TestUtil.assertEqualsVerbose(
+ TestUtil.fold(
+ "Type mismatch: ",
+ "f1: VARCHAR(10) NOT NULL -> VARCHAR(5) NOT NULL",
+ ""),
+ Util.toLinux(RelOptUtil.getFullTypeDifferenceString(t1, t2) + "\n"));
+
+ t2 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 4, 2)
+ .add("f1", SqlTypeName.BIGINT)
+ .build();
+
+ TestUtil.assertEqualsVerbose(
+ TestUtil.fold(
+ "Type mismatch: ",
+ "f0: DECIMAL(5, 2) NOT NULL -> DECIMAL(4, 2) NOT NULL",
+ "f1: VARCHAR(10) NOT NULL -> BIGINT NOT NULL",
+ ""),
+ Util.toLinux(RelOptUtil.getFullTypeDifferenceString(t1, t2) + "\n"));
+ }
Review comment:
Maybe we also need a test case for which the types are identical, and the difference string is empty?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r508171433
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType source,
+ RelDataType target) {
+ if (source == target) {
+ return "";
+ }
+
+ if (source.getFieldCount() > target.getFieldCount()) {
+ return "Type mismatch: the target type has less fields than the source type";
Review comment:
The error message is a hint to tell what may be wrong. From my experience, it is enough.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r510675864
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,57 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType source,
+ RelDataType target) {
+ if (source == target) {
+ return "";
+ }
+
+ final int sourceFieldCount = source.getFieldCount();
+ final int targetFieldCount = target.getFieldCount();
+ if (sourceFieldCount != targetFieldCount) {
+ return "Type mismatch: the field size of source and target are not equal, "
+ + "the source type has " + sourceFieldCount + " fields, "
+ + "while the target type has " + targetFieldCount + " fields";
Review comment:
Sounds reasonable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r507520376
##########
File path: core/src/test/java/org/apache/calcite/plan/RelOptUtilTest.java
##########
@@ -130,6 +130,43 @@
Util.toLinux(RelOptUtil.dumpType(t2) + "\n"));
}
+ @Test void testTypeDifference() {
+ final RelDataTypeFactory typeFactory =
+ new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+ final RelDataType t1 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 5, 2)
+ .add("f1", SqlTypeName.VARCHAR, 10)
+ .build();
+
+ RelDataType t2 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 5, 2)
+ .add("f1", SqlTypeName.VARCHAR, 5)
+ .build();
+
+ TestUtil.assertEqualsVerbose(
+ TestUtil.fold(
+ "Type mismatch: ",
+ "f1: VARCHAR(10) NOT NULL -> VARCHAR(5) NOT NULL",
+ ""),
+ Util.toLinux(RelOptUtil.getFullTypeDifferenceString(t1, t2) + "\n"));
+
+ t2 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 4, 2)
+ .add("f1", SqlTypeName.BIGINT)
+ .build();
+
+ TestUtil.assertEqualsVerbose(
+ TestUtil.fold(
+ "Type mismatch: ",
+ "f0: DECIMAL(5, 2) NOT NULL -> DECIMAL(4, 2) NOT NULL",
+ "f1: VARCHAR(10) NOT NULL -> BIGINT NOT NULL",
+ ""),
+ Util.toLinux(RelOptUtil.getFullTypeDifferenceString(t1, t2) + "\n"));
Review comment:
This is nice, however, can you please add a planning case as well to see which message comes out of a planner?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r510581259
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,57 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType source,
+ RelDataType target) {
+ if (source == target) {
+ return "";
+ }
+
+ final int sourceFieldCount = source.getFieldCount();
+ final int targetFieldCount = target.getFieldCount();
+ if (sourceFieldCount != targetFieldCount) {
+ return "Type mismatch: the field size of source and target are not equal, "
+ + "the source type has " + sourceFieldCount + " fields, "
+ + "while the target type has " + targetFieldCount + " fields";
Review comment:
+1, let's also keep the full type dump.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r509007111
##########
File path: core/src/test/java/org/apache/calcite/plan/RelOptUtilTest.java
##########
@@ -130,6 +130,43 @@
Util.toLinux(RelOptUtil.dumpType(t2) + "\n"));
}
+ @Test void testTypeDifference() {
+ final RelDataTypeFactory typeFactory =
+ new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+ final RelDataType t1 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 5, 2)
+ .add("f1", SqlTypeName.VARCHAR, 10)
+ .build();
+
+ RelDataType t2 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 5, 2)
+ .add("f1", SqlTypeName.VARCHAR, 5)
+ .build();
+
+ TestUtil.assertEqualsVerbose(
+ TestUtil.fold(
+ "Type mismatch: ",
+ "f1: VARCHAR(10) NOT NULL -> VARCHAR(5) NOT NULL",
+ ""),
+ Util.toLinux(RelOptUtil.getFullTypeDifferenceString(t1, t2) + "\n"));
+
+ t2 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 4, 2)
+ .add("f1", SqlTypeName.BIGINT)
+ .build();
+
+ TestUtil.assertEqualsVerbose(
+ TestUtil.fold(
+ "Type mismatch: ",
+ "f0: DECIMAL(5, 2) NOT NULL -> DECIMAL(4, 2) NOT NULL",
+ "f1: VARCHAR(10) NOT NULL -> BIGINT NOT NULL",
+ ""),
+ Util.toLinux(RelOptUtil.getFullTypeDifferenceString(t1, t2) + "\n"));
+ }
Review comment:
Makes sense.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r507569675
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType other,
+ RelDataType another) {
+ if (other == another) {
+ return "";
+ }
+
+ if (other.getFieldCount() != another.getFieldCount()) {
+ return "Type mismatch: the field size is not equal,"
+ + " one is " + other.getFieldCount()
+ + ", another one is " + another.getFieldCount();
+ }
Review comment:
Good idea.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r508350061
##########
File path: core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTest.java
##########
@@ -296,6 +300,32 @@
"PhysSingleRel:RelSubset#0.PHYS.[]")));
}
+ @Test void testTypeMismatch() {
+ VolcanoPlanner planner = new VolcanoPlanner();
+ planner.addRelTraitDef(ConventionTraitDef.INSTANCE);
+ planner.addRule(MockPhysLeafRule.INSTANCE);
+
+ RelOptCluster cluster = newCluster(planner);
+ NoneLeafRel leafRel =
+ new NoneLeafRel(
+ cluster,
+ "a");
+ RelNode convertedRel =
+ planner.changeTraits(
+ leafRel,
+ cluster.traitSetOf(PHYS_CALLING_CONVENTION));
+ planner.setRoot(convertedRel);
+ try {
+ planner.chooseDelegate().findBestExp();
+ } catch (RuntimeException e) {
Review comment:
Please use `assertThrows` to make sure that an exception is actually thrown.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r507717906
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType other,
+ RelDataType another) {
+ if (other == another) {
+ return "";
+ }
+
+ if (other.getFieldCount() != another.getFieldCount()) {
+ return "Type mismatch: the field size is not equal,"
+ + " one is " + other.getFieldCount()
+ + ", another one is " + another.getFieldCount();
+ }
+
+ final StringBuilder stringBuilder = new StringBuilder("Type mismatch: \n");
+ final List<RelDataTypeField> f1 = other.getFieldList();
+ final List<RelDataTypeField> f2 = another.getFieldList();
+ for (Pair<RelDataTypeField, RelDataTypeField> pair : Pair.zip(f1, f2)) {
+ final RelDataType type1 = pair.left.getType();
+ final RelDataType type2 = pair.right.getType();
+ // If one of the types is ANY comparison should succeed
+ if (type1.getSqlTypeName() == SqlTypeName.ANY
+ || type2.getSqlTypeName() == SqlTypeName.ANY) {
Review comment:
They are a bit different. `areRowTypesEqual` would quit immediately if two fields are not equal, while `getFullTypeDifferenceString` would find all the different fields.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r507569978
##########
File path: core/src/test/java/org/apache/calcite/plan/RelOptUtilTest.java
##########
@@ -130,6 +130,43 @@
Util.toLinux(RelOptUtil.dumpType(t2) + "\n"));
}
+ @Test void testTypeDifference() {
+ final RelDataTypeFactory typeFactory =
+ new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+ final RelDataType t1 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 5, 2)
+ .add("f1", SqlTypeName.VARCHAR, 10)
+ .build();
+
+ RelDataType t2 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 5, 2)
+ .add("f1", SqlTypeName.VARCHAR, 5)
+ .build();
+
+ TestUtil.assertEqualsVerbose(
+ TestUtil.fold(
+ "Type mismatch: ",
+ "f1: VARCHAR(10) NOT NULL -> VARCHAR(5) NOT NULL",
+ ""),
+ Util.toLinux(RelOptUtil.getFullTypeDifferenceString(t1, t2) + "\n"));
+
+ t2 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 4, 2)
+ .add("f1", SqlTypeName.BIGINT)
+ .build();
+
+ TestUtil.assertEqualsVerbose(
+ TestUtil.fold(
+ "Type mismatch: ",
+ "f0: DECIMAL(5, 2) NOT NULL -> DECIMAL(4, 2) NOT NULL",
+ "f1: VARCHAR(10) NOT NULL -> BIGINT NOT NULL",
+ ""),
+ Util.toLinux(RelOptUtil.getFullTypeDifferenceString(t1, t2) + "\n"));
Review comment:
With pleasure.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r507676637
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
Review comment:
Is this method used anywhere?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r508344078
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType source,
+ RelDataType target) {
+ if (source == target) {
+ return "";
+ }
+
+ if (source.getFieldCount() > target.getFieldCount()) {
+ return "Type mismatch: the target type has less fields than the source type";
Review comment:
Agreed with @vlsi , it may be helpful to provide the field count.
If so, there is no need to distinguish the cases for source count > target count and source count < target count.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] chunweilei commented on a change in pull request #2224: [CALCITE-4332] Improve error when planning rule produces a relational…
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2224:
URL: https://github.com/apache/calcite/pull/2224#discussion_r507569800
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -2199,13 +2199,49 @@ public static boolean equal(
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
+ return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}\ndifference:\n{}",
desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ desc2, type2.getFullTypeString(),
+ getFullTypeDifferenceString(type1, type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(RelDataType other,
+ RelDataType another) {
+ if (other == another) {
+ return "";
+ }
+
+ if (other.getFieldCount() != another.getFieldCount()) {
+ return "Type mismatch: the field size is not equal,"
+ + " one is " + other.getFieldCount()
+ + ", another one is " + another.getFieldCount();
Review comment:
Sounds reasonable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org