From 91e6c139dd65359cb371edf3ed2f4a2119c91c85 Mon Sep 17 00:00:00 2001 From: Soumyakanti Das Date: Mon, 8 Jul 2024 11:55:47 -0700 Subject: [PATCH 1/2] [CALCITE-6448] FilterReduceExpressionsRule returns empty RelNode for RexLiterals --- .../org/apache/calcite/rel/core/Filter.java | 3 + .../org/apache/calcite/rel/core/Join.java | 2 + .../apache/calcite/test/RelBuilderTest.java | 19 +++--- .../org/apache/calcite/test/PigRelExTest.java | 68 +++++++++---------- 4 files changed, 47 insertions(+), 45 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/rel/core/Filter.java b/core/src/main/java/org/apache/calcite/rel/core/Filter.java index 14b4b2fcc27..8d7bf74a1d5 100644 --- a/core/src/main/java/org/apache/calcite/rel/core/Filter.java +++ b/core/src/main/java/org/apache/calcite/rel/core/Filter.java @@ -34,6 +34,7 @@ import org.apache.calcite.rex.RexProgram; import org.apache.calcite.rex.RexShuttle; import org.apache.calcite.rex.RexUtil; +import org.apache.calcite.sql.type.SqlTypeName; import org.apache.calcite.util.Litmus; import com.google.common.collect.ImmutableList; @@ -85,6 +86,8 @@ protected Filter( RexNode condition) { super(cluster, traits, child); this.condition = requireNonNull(condition, "condition"); + assert SqlTypeName.BOOLEAN == condition.getType().getSqlTypeName() + : "condition should be of BOOLEAN type, but was " + condition.getType(); assert RexUtil.isFlat(condition) : "RexUtil.isFlat should be true for condition " + condition; assert isValid(Litmus.THROW, null); this.hints = ImmutableList.copyOf(hints); diff --git a/core/src/main/java/org/apache/calcite/rel/core/Join.java b/core/src/main/java/org/apache/calcite/rel/core/Join.java index dc99bfbfe04..13d3a4c1c7c 100644 --- a/core/src/main/java/org/apache/calcite/rel/core/Join.java +++ b/core/src/main/java/org/apache/calcite/rel/core/Join.java @@ -100,6 +100,8 @@ protected Join( JoinRelType joinType) { super(cluster, traitSet, left, right); this.condition = Objects.requireNonNull(condition, "condition"); + assert SqlTypeName.BOOLEAN == condition.getType().getSqlTypeName() + : "condition should be of BOOLEAN type, but was " + condition.getType(); this.variablesSet = ImmutableSet.copyOf(variablesSet); this.joinType = Objects.requireNonNull(joinType, "joinType"); this.joinInfo = JoinInfo.of(left, right, condition); diff --git a/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java b/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java index e71b816bc89..492c95ccf35 100644 --- a/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java +++ b/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java @@ -2650,19 +2650,17 @@ private static RelBuilder assertSize(RelBuilder b, b.scan("EMP") .scan("DEPT") .join(JoinRelType.INNER, - b.or(b.literal(null), - b.and(b.equals(b.field(2, 0, "DEPTNO"), b.literal(1)), - b.equals(b.field(2, 0, "DEPTNO"), b.literal(2)), - b.equals(b.field(2, 1, "DEPTNO"), - b.field(2, 0, "DEPTNO"))))) + b.and(b.equals(b.field(2, 0, "DEPTNO"), b.literal(1)), + b.equals(b.field(2, 0, "DEPTNO"), b.literal(2)), + b.equals(b.field(2, 1, "DEPTNO"), + b.field(2, 0, "DEPTNO")))) .build(); final String expected = "" + "LogicalJoin(condition=[false], joinType=[inner])\n" + " LogicalTableScan(table=[[scott, EMP]])\n" + " LogicalTableScan(table=[[scott, DEPT]])\n"; final String expectedWithoutSimplify = "" - + "LogicalJoin(condition=[OR(null:NULL, " - + "AND(=($7, 1), =($7, 2), =($8, $7)))], joinType=[inner])\n" + + "LogicalJoin(condition=[AND(=($7, 1), =($7, 2), =($8, $7))], joinType=[inner])\n" + " LogicalTableScan(table=[[scott, EMP]])\n" + " LogicalTableScan(table=[[scott, DEPT]])\n"; assertThat(f.apply(createBuilder()), hasTree(expected)); @@ -4512,13 +4510,12 @@ private static RelBuilder assertSize(RelBuilder b, Function f = b -> b.scan("EMP") .filter( - b.or(b.literal(null), - b.and(b.equals(b.field(2), b.literal(1)), - b.equals(b.field(2), b.literal(2))))) + b.and(b.equals(b.field(2), b.literal(1)), + b.equals(b.field(2), b.literal(2)))) .build(); final String expected = "LogicalValues(tuples=[[]])\n"; final String expectedWithoutSimplify = "" - + "LogicalFilter(condition=[OR(null:NULL, AND(=($2, 1), =($2, 2)))])\n" + + "LogicalFilter(condition=[AND(=($2, 1), =($2, 2))])\n" + " LogicalTableScan(table=[[scott, EMP]])\n"; assertThat(f.apply(createBuilder()), hasTree(expected)); assertThat(f.apply(createBuilder(c -> c.withSimplify(false))), diff --git a/piglet/src/test/java/org/apache/calcite/test/PigRelExTest.java b/piglet/src/test/java/org/apache/calcite/test/PigRelExTest.java index b36f684fa59..61083ad94aa 100644 --- a/piglet/src/test/java/org/apache/calcite/test/PigRelExTest.java +++ b/piglet/src/test/java/org/apache/calcite/test/PigRelExTest.java @@ -145,76 +145,76 @@ public void testMatch() { } @Test void testAdd() { - checkTranslation("b + 3", inTree("+($1, 3)")); + checkTranslation("(boolean)(b + 3)", inTree("+($1, 3)")); } @Test void testSubtract() { - checkTranslation("b - 3", inTree("-($1, 3)")); + checkTranslation("(boolean)(b - 3)", inTree("-($1, 3)")); } @Test void testMultiply() { - checkTranslation("b * 3", inTree("*($1, 3)")); + checkTranslation("(boolean)(b * 3)", inTree("*($1, 3)")); } @Test void testMod() { - checkTranslation("b % 3", inTree("MOD($1, 3)")); + checkTranslation("(boolean)(b % 3)", inTree("MOD($1, 3)")); } @Test void testDivide() { - checkTranslation("b / 3", inTree("/($1, 3)")); - checkTranslation("c / 3.1", inTree("/($2, 3.1E0:DOUBLE)")); + checkTranslation("(boolean)(b / 3)", inTree("/($1, 3)")); + checkTranslation("(boolean)(c / 3.1)", inTree("/($2, 3.1E0:DOUBLE)")); } @Test void testBinCond() { - checkTranslation("(b == 1 ? 2 : 3)", inTree("CASE(=($1, 1), 2, 3)")); + checkTranslation("(boolean)(b == 1 ? 2 : 3)", inTree("CASE(=($1, 1), 2, 3)")); } @Test void testTupleDereference() { - checkTranslation("k2.k21", inTree("[$11.k21]")); - checkTranslation("k2.(k21, k22)", inTree("[ROW($11.k21, $11.k22)]")); - checkTranslation("k2.k22.(k221,k222)", - inTree("[ROW($11.k22.k221, $11.k22.k222)]")); + checkTranslation("(boolean)k2.k21", inTree("$11.k21")); + checkTranslation("(boolean)k2.(k21, k22)", inTree("ROW($11.k21, $11.k22)")); + checkTranslation("(boolean)k2.k22.(k221,k222)", + inTree("ROW($11.k22.k221, $11.k22.k222)")); } @Test void testBagDereference() { - checkTranslation("l2.l22", inTree("[MULTISET_PROJECTION($13, 1)]")); - checkTranslation("l2.(l21, l22)", inTree("[MULTISET_PROJECTION($13, 0, 1)]")); + checkTranslation("(boolean)l2.l22", inTree("MULTISET_PROJECTION($13, 1)")); + checkTranslation("(boolean)l2.(l21, l22)", inTree("MULTISET_PROJECTION($13, 0, 1)")); } @Test void testMapLookup() { - checkTranslation("m2#'testKey'", inTree("ITEM($15, 'testKey')")); + checkTranslation("(boolean)(m2#'testKey')", inTree("ITEM($15, 'testKey')")); } @Test void testCast() { - checkTranslation("(int) b", inTree("CAST($1):INTEGER")); - checkTranslation("(long) a", inTree("CAST($0):BIGINT")); - checkTranslation("(float) b", inTree("CAST($1):REAL")); - checkTranslation("(double) b", inTree("CAST($1):DOUBLE")); - checkTranslation("(chararray) b", inTree("CAST($1):VARCHAR")); - checkTranslation("(bytearray) b", inTree("CAST($1):BINARY")); - checkTranslation("(boolean) c", inTree("CAST($2):BOOLEAN")); - checkTranslation("(biginteger) b", inTree("CAST($1):DECIMAL(19, 0)")); - checkTranslation("(bigdecimal) b", inTree("CAST($1):DECIMAL(19, 0)")); - checkTranslation("(tuple()) b", inTree("CAST($1):(DynamicRecordRow[])")); - checkTranslation("(tuple(int, float)) b", + checkTranslation("(boolean)(int) b", inTree("CAST($1):INTEGER")); + checkTranslation("(boolean)(long) a", inTree("CAST($0):BIGINT")); + checkTranslation("(boolean)(float) b", inTree("CAST($1):REAL")); + checkTranslation("(boolean)(double) b", inTree("CAST($1):DOUBLE")); + checkTranslation("(boolean)(chararray) b", inTree("CAST($1):VARCHAR")); + checkTranslation("(boolean)(bytearray) b", inTree("CAST($1):BINARY")); + checkTranslation("(boolean)(boolean) c", inTree("CAST($2):BOOLEAN")); + checkTranslation("(boolean)(biginteger) b", inTree("CAST($1):DECIMAL(19, 0)")); + checkTranslation("(boolean)(bigdecimal) b", inTree("CAST($1):DECIMAL(19, 0)")); + checkTranslation("(boolean)(tuple()) b", inTree("CAST($1):(DynamicRecordRow[])")); + checkTranslation("(boolean)(tuple(int, float)) b", inTree("CAST($1):RecordType(INTEGER $0, REAL $1)")); - checkTranslation("(bag{}) b", + checkTranslation("(boolean)(bag{}) b", inTree("CAST($1):(DynamicRecordRow[]) NOT NULL MULTISET")); - checkTranslation("(bag{tuple(int)}) b", + checkTranslation("(boolean)(bag{tuple(int)}) b", inTree("CAST($1):RecordType(INTEGER $0) MULTISET")); - checkTranslation("(bag{tuple(int, float)}) b", + checkTranslation("(boolean)(bag{tuple(int, float)}) b", inTree("CAST($1):RecordType(INTEGER $0, REAL $1) MULTISET")); - checkTranslation("(map[]) b", + checkTranslation("(boolean)(map[]) b", inTree("CAST($1):(VARCHAR NOT NULL, BINARY(1) NOT NULL) MAP")); - checkTranslation("(map[int]) b", inTree("CAST($1):(VARCHAR NOT NULL, INTEGER")); - checkTranslation("(map[tuple(int, float)]) b", + checkTranslation("(boolean)(map[int]) b", inTree("CAST($1):(VARCHAR NOT NULL, INTEGER")); + checkTranslation("(boolean)(map[tuple(int, float)]) b", inTree("CAST($1):(VARCHAR NOT NULL, RecordType(INTEGER val_0, REAL val_1)) MAP")); } @Test void testPigBuiltinFunctions() { - checkTranslation("ABS(-5)", inTree("ABS(-5)")); - checkTranslation("AddDuration(h, 'P1D')", + checkTranslation("(boolean)ABS(-5)", inTree("ABS(-5)")); + checkTranslation("(boolean)AddDuration(h, 'P1D')", inTree("AddDuration(PIG_TUPLE($7, 'P1D'))")); - checkTranslation("CEIL(1.2)", inTree("CEIL(1.2E0:DOUBLE)")); + checkTranslation("(boolean)CEIL(1.2)", inTree("CEIL(1.2E0:DOUBLE)")); } } From e21b7c9a583b51c464cc5d7ef5669b62be81ffc5 Mon Sep 17 00:00:00 2001 From: Soumyakanti Das Date: Fri, 19 Jul 2024 11:22:03 -0700 Subject: [PATCH 2/2] move checks to isValid method --- core/src/main/java/org/apache/calcite/rel/core/Filter.java | 5 +++-- core/src/main/java/org/apache/calcite/rel/core/Join.java | 3 +-- .../java/org/apache/calcite/rel/logical/LogicalJoin.java | 3 ++- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/rel/core/Filter.java b/core/src/main/java/org/apache/calcite/rel/core/Filter.java index 8d7bf74a1d5..498ab9b0a07 100644 --- a/core/src/main/java/org/apache/calcite/rel/core/Filter.java +++ b/core/src/main/java/org/apache/calcite/rel/core/Filter.java @@ -86,8 +86,6 @@ protected Filter( RexNode condition) { super(cluster, traits, child); this.condition = requireNonNull(condition, "condition"); - assert SqlTypeName.BOOLEAN == condition.getType().getSqlTypeName() - : "condition should be of BOOLEAN type, but was " + condition.getType(); assert RexUtil.isFlat(condition) : "RexUtil.isFlat should be true for condition " + condition; assert isValid(Litmus.THROW, null); this.hints = ImmutableList.copyOf(hints); @@ -146,6 +144,9 @@ public final boolean containsOver() { } @Override public boolean isValid(Litmus litmus, @Nullable Context context) { + if (condition.getType().getSqlTypeName() != SqlTypeName.BOOLEAN) { + return litmus.fail("condition must be boolean: {}", condition.getType()); + } if (RexUtil.isNullabilityCast(getCluster().getTypeFactory(), condition)) { return litmus.fail("Cast for just nullability not allowed"); } diff --git a/core/src/main/java/org/apache/calcite/rel/core/Join.java b/core/src/main/java/org/apache/calcite/rel/core/Join.java index 13d3a4c1c7c..5d8ebedcfc6 100644 --- a/core/src/main/java/org/apache/calcite/rel/core/Join.java +++ b/core/src/main/java/org/apache/calcite/rel/core/Join.java @@ -100,12 +100,11 @@ protected Join( JoinRelType joinType) { super(cluster, traitSet, left, right); this.condition = Objects.requireNonNull(condition, "condition"); - assert SqlTypeName.BOOLEAN == condition.getType().getSqlTypeName() - : "condition should be of BOOLEAN type, but was " + condition.getType(); this.variablesSet = ImmutableSet.copyOf(variablesSet); this.joinType = Objects.requireNonNull(joinType, "joinType"); this.joinInfo = JoinInfo.of(left, right, condition); this.hints = ImmutableList.copyOf(hints); + assert isValid(Litmus.THROW, null); } @Deprecated // to be removed before 2.0 diff --git a/core/src/main/java/org/apache/calcite/rel/logical/LogicalJoin.java b/core/src/main/java/org/apache/calcite/rel/logical/LogicalJoin.java index c369fa8e042..3d7f47ead85 100644 --- a/core/src/main/java/org/apache/calcite/rel/logical/LogicalJoin.java +++ b/core/src/main/java/org/apache/calcite/rel/logical/LogicalJoin.java @@ -36,6 +36,7 @@ import org.checkerframework.checker.nullness.qual.Nullable; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Objects; import java.util.Set; @@ -215,7 +216,7 @@ public static LogicalJoin create(RelNode left, RelNode right, List hint } @Override public List getSystemFieldList() { - return systemFieldList; + return systemFieldList != null ? systemFieldList : Collections.emptyList(); } @Override public RelNode withHints(List hintList) {