This is an automated email from the ASF dual-hosted git repository.
chunwei pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/calcite.git
The following commit(s) were added to refs/heads/master by this push:
new f9d23a6 [CALCITE-4332] Improve error when planning rule produces a
relational expression with wrong row type
f9d23a6 is described below
commit f9d23a62145e8c490492214836ce9cd7d1dcc35e
Author: Chunwei Lei <[email protected]>
AuthorDate: Mon Oct 19 14:03:48 2020 +0800
[CALCITE-4332] Improve error when planning rule produces a relational
expression with wrong row type
---
.../java/org/apache/calcite/plan/RelOptUtil.java | 54 +++++++++++++++++-
.../calcite/plan/volcano/VolcanoPlanner.java | 25 ++++----
.../org/apache/calcite/plan/RelOptUtilTest.java | 66 ++++++++++++++++++++++
.../apache/calcite/plan/volcano/PlannerTests.java | 46 +++++++++++++++
.../calcite/plan/volcano/VolcanoPlannerTest.java | 35 ++++++++++++
5 files changed, 213 insertions(+), 13 deletions(-)
diff --git a/core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
b/core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
index e231387..54575d7 100644
--- a/core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
+++ b/core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
@@ -2199,13 +2199,61 @@ public abstract class RelOptUtil {
RelDataType type2,
Litmus litmus) {
if (!areRowTypesEqual(type1, type2, false)) {
- return litmus.fail("Type mismatch:\n{}:\n{}\n{}:\n{}",
- desc1, type1.getFullTypeString(),
- desc2, type2.getFullTypeString());
+ return litmus.fail(getFullTypeDifferenceString(desc1, type1, desc2,
type2));
}
return litmus.succeed();
}
+ public static String getFullTypeDifferenceString(
+ final String sourceDesc,
+ RelDataType sourceType,
+ final String targetDesc,
+ RelDataType targetType
+ ) {
+ if (sourceType == targetType) {
+ return "";
+ }
+
+ final int sourceFieldCount = sourceType.getFieldCount();
+ final int targetFieldCount = targetType.getFieldCount();
+ if (sourceFieldCount != targetFieldCount) {
+ return "Type mismatch: the field sizes are not equal.\n"
+ + sourceDesc + ": " + sourceType.getFullTypeString() + "\n"
+ + targetDesc + ": " + targetType.getFullTypeString();
+ }
+
+ final StringBuilder stringBuilder = new StringBuilder();
+ final List<RelDataTypeField> f1 = sourceType.getFieldList();
+ final List<RelDataTypeField> f2 = targetType.getFieldList();
+ for (Pair<RelDataTypeField, RelDataTypeField> pair : Pair.zip(f1, f2)) {
+ final RelDataType t1 = pair.left.getType();
+ final RelDataType t2 = pair.right.getType();
+ // If one of the types is ANY comparison should succeed
+ if (sourceType.getSqlTypeName() == SqlTypeName.ANY
+ || targetType.getSqlTypeName() == SqlTypeName.ANY) {
+ continue;
+ }
+ if (!t1.equals(t2)) {
+ stringBuilder.append(pair.left.getName());
+ stringBuilder.append(": ");
+ stringBuilder.append(t1.getFullTypeString());
+ stringBuilder.append(" -> ");
+ stringBuilder.append(t2.getFullTypeString());
+ stringBuilder.append("\n");
+ }
+ }
+ final String difference = stringBuilder.toString();
+ if (!difference.isEmpty()) {
+ return "Type mismatch:\n"
+ + sourceDesc + ": " + sourceType.getFullTypeString() + "\n"
+ + targetDesc + ": " + targetType.getFullTypeString() + "\n"
+ + "Difference:\n"
+ + difference;
+ } else {
+ return "";
+ }
+ }
+
/** Returns whether two relational expressions have the same row-type. */
public static boolean equalType(String desc0, RelNode rel0, String desc1,
RelNode rel1, Litmus litmus) {
diff --git
a/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
b/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
index 85474b9..32d647d 100644
--- a/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
+++ b/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
@@ -50,6 +50,7 @@ import org.apache.calcite.rel.metadata.RelMetadataProvider;
import org.apache.calcite.rel.metadata.RelMetadataQuery;
import org.apache.calcite.rel.rules.SubstitutionRule;
import org.apache.calcite.rel.rules.TransformationRule;
+import org.apache.calcite.rel.type.RelDataType;
import org.apache.calcite.runtime.Hook;
import org.apache.calcite.sql.SqlExplainLevel;
import org.apache.calcite.util.Litmus;
@@ -578,12 +579,14 @@ public class VolcanoPlanner extends AbstractRelOptPlanner
{
if (equivRel == null) {
set = null;
} else {
- assert RelOptUtil.equal(
- "rel rowtype",
- rel.getRowType(),
- "equivRel rowtype",
- equivRel.getRowType(),
- Litmus.THROW);
+ final RelDataType relType = rel.getRowType();
+ final RelDataType equivRelType = equivRel.getRowType();
+ if (!RelOptUtil.areRowTypesEqual(relType,
+ equivRelType, false)) {
+ throw new IllegalArgumentException(
+ RelOptUtil.getFullTypeDifferenceString("rel rowtype", relType,
+ "equiv rowtype", equivRelType));
+ }
equivRel = ensureRegistered(equivRel, null);
set = getSet(equivRel);
}
@@ -1211,10 +1214,12 @@ public class VolcanoPlanner extends
AbstractRelOptPlanner {
} else if (equivExp == rel) {
return getSubset(rel);
} else {
- assert RelOptUtil.equal(
- "left", equivExp.getRowType(),
- "right", rel.getRowType(),
- Litmus.THROW);
+ if (!RelOptUtil.areRowTypesEqual(equivExp.getRowType(),
+ rel.getRowType(), false)) {
+ throw new IllegalArgumentException(
+ RelOptUtil.getFullTypeDifferenceString("equiv rowtype",
+ equivExp.getRowType(), "rel rowtype", rel.getRowType()));
+ }
checkPruned(equivExp, rel);
RelSet equivSet = getSet(equivExp);
diff --git a/core/src/test/java/org/apache/calcite/plan/RelOptUtilTest.java
b/core/src/test/java/org/apache/calcite/plan/RelOptUtilTest.java
index 2fa4015..bc02f7f 100644
--- a/core/src/test/java/org/apache/calcite/plan/RelOptUtilTest.java
+++ b/core/src/test/java/org/apache/calcite/plan/RelOptUtilTest.java
@@ -59,6 +59,7 @@ import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
+import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.instanceOf;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
@@ -130,6 +131,71 @@ class RelOptUtilTest {
Util.toLinux(RelOptUtil.dumpType(t2) + "\n"));
}
+ @Test void testTypeDifference() {
+ final RelDataTypeFactory typeFactory =
+ new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+
+ final RelDataType t0 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 5, 2)
+ .build();
+
+ final RelDataType t1 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 5, 2)
+ .add("f1", SqlTypeName.VARCHAR, 10)
+ .build();
+
+ TestUtil.assertEqualsVerbose(
+ TestUtil.fold(
+ "Type mismatch: the field sizes are not equal.",
+ "source: RecordType(DECIMAL(5, 2) NOT NULL f0) NOT NULL",
+ "target: RecordType(DECIMAL(5, 2) NOT NULL f0, VARCHAR(10) NOT
NULL f1) NOT NULL"),
+ Util.toLinux(RelOptUtil.getFullTypeDifferenceString("source", t0,
"target", t1) + "\n"));
+
+ RelDataType t2 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 5, 2)
+ .add("f1", SqlTypeName.VARCHAR, 5)
+ .build();
+
+ TestUtil.assertEqualsVerbose(
+ TestUtil.fold(
+ "Type mismatch:",
+ "source: RecordType(DECIMAL(5, 2) NOT NULL f0, VARCHAR(10) NOT
NULL f1) NOT NULL",
+ "target: RecordType(DECIMAL(5, 2) NOT NULL f0, VARCHAR(5) NOT NULL
f1) NOT NULL",
+ "Difference:",
+ "f1: VARCHAR(10) NOT NULL -> VARCHAR(5) NOT NULL",
+ ""),
+ Util.toLinux(RelOptUtil.getFullTypeDifferenceString("source", t1,
"target", t2) + "\n"));
+
+ t2 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 4, 2)
+ .add("f1", SqlTypeName.BIGINT)
+ .build();
+
+ TestUtil.assertEqualsVerbose(
+ TestUtil.fold(
+ "Type mismatch:",
+ "source: RecordType(DECIMAL(5, 2) NOT NULL f0, VARCHAR(10) NOT
NULL f1) NOT NULL",
+ "target: RecordType(DECIMAL(4, 2) NOT NULL f0, BIGINT NOT NULL f1)
NOT NULL",
+ "Difference:",
+ "f0: DECIMAL(5, 2) NOT NULL -> DECIMAL(4, 2) NOT NULL",
+ "f1: VARCHAR(10) NOT NULL -> BIGINT NOT NULL",
+ ""),
+ Util.toLinux(RelOptUtil.getFullTypeDifferenceString("source", t1,
"target", t2) + "\n"));
+
+ t2 =
+ typeFactory.builder()
+ .add("f0", SqlTypeName.DECIMAL, 5, 2)
+ .add("f1", SqlTypeName.VARCHAR, 10)
+ .build();
+ // Test identical types.
+ assertThat(RelOptUtil.getFullTypeDifferenceString("source", t1, "target",
t2), equalTo(""));
+ assertThat(RelOptUtil.getFullTypeDifferenceString("source", t1, "target",
t1), equalTo(""));
+ }
+
/**
* Tests the rules for how we name rules.
*/
diff --git
a/core/src/test/java/org/apache/calcite/plan/volcano/PlannerTests.java
b/core/src/test/java/org/apache/calcite/plan/volcano/PlannerTests.java
index 7c47dcc..e539130f 100644
--- a/core/src/test/java/org/apache/calcite/plan/volcano/PlannerTests.java
+++ b/core/src/test/java/org/apache/calcite/plan/volcano/PlannerTests.java
@@ -246,6 +246,52 @@ class PlannerTests {
}
}
+ /** Planner rule that converts {@link NoneLeafRel} to PHYS convention with
different type. */
+ public static class MockPhysLeafRule extends
RelRule<MockPhysLeafRule.Config> {
+ static final MockPhysLeafRule INSTANCE =
+ Config.EMPTY
+ .withOperandSupplier(b -> b.operand(NoneLeafRel.class).anyInputs())
+ .as(Config.class)
+ .toRule();
+
+ /** Relational expression with zero inputs and convention PHYS. */
+ public static class MockPhysLeafRel extends PhysLeafRel {
+ MockPhysLeafRel(RelOptCluster cluster, String label) {
+ super(cluster, PHYS_CALLING_CONVENTION, label);
+ }
+
+ @Override protected RelDataType deriveRowType() {
+ final RelDataTypeFactory typeFactory = getCluster().getTypeFactory();
+ return typeFactory.builder()
+ .add("this", typeFactory.createJavaType(Integer.class))
+ .build();
+ }
+ }
+
+ protected MockPhysLeafRule(Config config) {
+ super(config);
+ }
+
+ @Override public Convention getOutConvention() {
+ return PHYS_CALLING_CONVENTION;
+ }
+
+ @Override public void onMatch(RelOptRuleCall call) {
+ NoneLeafRel leafRel = call.rel(0);
+
+ // It would throw exception.
+ call.transformTo(
+ new MockPhysLeafRel(leafRel.getCluster(), leafRel.label));
+ }
+
+ /** Rule configuration. */
+ public interface Config extends RelRule.Config {
+ @Override default MockPhysLeafRule toRule() {
+ return new MockPhysLeafRule(this);
+ }
+ }
+ }
+
/** Planner rule that matches a {@link NoneSingleRel} and succeeds. */
public static class GoodSingleRule
extends RelRule<GoodSingleRule.Config> {
diff --git
a/core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTest.java
b/core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTest.java
index 1fc61f2..645a878 100644
--- a/core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTest.java
+++ b/core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTest.java
@@ -43,6 +43,8 @@ import org.apache.calcite.tools.RelBuilder;
import org.apache.calcite.util.ImmutableBeans;
import org.apache.calcite.util.Pair;
+import org.apache.commons.lang.exception.ExceptionUtils;
+
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
@@ -55,6 +57,7 @@ import java.util.List;
import static
org.apache.calcite.plan.volcano.PlannerTests.AssertOperandsDifferentRule;
import static org.apache.calcite.plan.volcano.PlannerTests.GoodSingleRule;
+import static org.apache.calcite.plan.volcano.PlannerTests.MockPhysLeafRule;
import static org.apache.calcite.plan.volcano.PlannerTests.NoneLeafRel;
import static org.apache.calcite.plan.volcano.PlannerTests.NoneSingleRel;
import static
org.apache.calcite.plan.volcano.PlannerTests.PHYS_CALLING_CONVENTION;
@@ -69,10 +72,12 @@ import static
org.apache.calcite.plan.volcano.PlannerTests.newCluster;
import static org.apache.calcite.test.Matchers.isLinux;
import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.instanceOf;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
/**
@@ -296,6 +301,36 @@ class VolcanoPlannerTest {
"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);
+
+ RuntimeException ex = assertThrows(RuntimeException.class, () -> {
+ planner.chooseDelegate().findBestExp();
+ }, "Should throw exception fail since the type mismatches after applying
rule.");
+
+ Throwable exception = ExceptionUtils.getRootCause(ex);
+ assertThat(exception, instanceOf(IllegalArgumentException.class));
+ assertThat(
+ exception.getMessage(), isLinux("Type mismatch:\n"
+ + "rel rowtype: RecordType(JavaType(class java.lang.Integer) this)
NOT NULL\n"
+ + "equiv rowtype: RecordType(JavaType(void) NOT NULL this) NOT
NULL\n"
+ + "Difference:\n"
+ + "this: JavaType(class java.lang.Integer) -> JavaType(void) NOT
NULL\n"));
+ }
+
private static <E extends Comparable> List<E> sort(List<E> list) {
final List<E> list2 = new ArrayList<>(list);
Collections.sort(list2);
