Repository: calcite Updated Branches: refs/heads/branch-1.18 5f07b88df -> b3d144bdf (forced update)
[CALCITE-2726] ReduceExpressionRule oversimplifies filter conditions containing nulls ReduceExpressionsRule might have simplified "(empno=null and mgr=1) is null" to "false". Add test case based on [HIVE-20617]; deprecate ExprSimplifier (Julian Hyde) Close apache/calcite#956 Project: http://git-wip-us.apache.org/repos/asf/calcite/repo Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/4da9c0d9 Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/4da9c0d9 Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/4da9c0d9 Branch: refs/heads/branch-1.18 Commit: 4da9c0d94ee0e2a2f4d03845730ffc63a83a7cbd Parents: efec74d Author: Zoltan Haindrich <k...@rxd.hu> Authored: Wed Dec 5 16:32:50 2018 +0100 Committer: Julian Hyde <jh...@apache.org> Committed: Wed Dec 5 16:18:19 2018 -0800 ---------------------------------------------------------------------- .../rel/rules/ReduceExpressionsRule.java | 9 ++-- .../org/apache/calcite/rex/RexSimplify.java | 7 +-- .../java/org/apache/calcite/rex/RexUtil.java | 8 ++-- .../apache/calcite/test/RelOptRulesTest.java | 15 ++++++ .../calcite/test/RexImplicationCheckerTest.java | 50 ++++++++++++-------- .../org/apache/calcite/test/RelOptRulesTest.xml | 23 ++++++++- core/src/test/resources/sql/conditions.iq | 13 +++++ 7 files changed, 93 insertions(+), 32 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/calcite/blob/4da9c0d9/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java b/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java index 50823f4..48ed3cd 100644 --- a/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java +++ b/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java @@ -54,7 +54,6 @@ import org.apache.calcite.rex.RexSimplify; import org.apache.calcite.rex.RexSubQuery; import org.apache.calcite.rex.RexUnknownAs; import org.apache.calcite.rex.RexUtil; -import org.apache.calcite.rex.RexUtil.ExprSimplifier; import org.apache.calcite.rex.RexVisitorImpl; import org.apache.calcite.sql.SqlKind; import org.apache.calcite.sql.SqlOperator; @@ -539,14 +538,14 @@ public abstract class ReduceExpressionsRule extends RelOptRule { // Simplify predicates in place final RexUnknownAs unknownAs = RexUnknownAs.falseIf(unknownAsFalse); - boolean reduced = reduceExpressionsInternal(rel, simplify, unknownAs, + final boolean reduced = reduceExpressionsInternal(rel, simplify, unknownAs, expList, predicates); - final ExprSimplifier simplifier = - new ExprSimplifier(simplify, unknownAs, matchNullability); boolean simplified = false; for (int i = 0; i < expList.size(); i++) { - RexNode expr2 = simplifier.apply(expList.get(i)); + final RexNode expr2 = + simplify.simplifyPreservingType(expList.get(i), unknownAs, + matchNullability); if (!expr2.equals(expList.get(i))) { expList.set(i, expr2); simplified = true; http://git-wip-us.apache.org/repos/asf/calcite/blob/4da9c0d9/core/src/main/java/org/apache/calcite/rex/RexSimplify.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java b/core/src/main/java/org/apache/calcite/rex/RexSimplify.java index f78a90c..c7b6dac 100644 --- a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java +++ b/core/src/main/java/org/apache/calcite/rex/RexSimplify.java @@ -164,15 +164,16 @@ public class RexSimplify { * <p>This is useful if you are simplifying expressions in a * {@link Project}. */ public RexNode simplifyPreservingType(RexNode e) { - return simplifyPreservingType(e, defaultUnknownAs); + return simplifyPreservingType(e, defaultUnknownAs, true); } - private RexNode simplifyPreservingType(RexNode e, RexUnknownAs unknownAs) { + public RexNode simplifyPreservingType(RexNode e, RexUnknownAs unknownAs, + boolean matchNullability) { final RexNode e2 = simplifyUnknownAs(e, unknownAs); if (e2.getType() == e.getType()) { return e2; } - final RexNode e3 = rexBuilder.makeCast(e.getType(), e2, true); + final RexNode e3 = rexBuilder.makeCast(e.getType(), e2, matchNullability); if (e3.equals(e)) { return e; } http://git-wip-us.apache.org/repos/asf/calcite/blob/4da9c0d9/core/src/main/java/org/apache/calcite/rex/RexUtil.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/rex/RexUtil.java b/core/src/main/java/org/apache/calcite/rex/RexUtil.java index 56eabb6..053d950 100644 --- a/core/src/main/java/org/apache/calcite/rex/RexUtil.java +++ b/core/src/main/java/org/apache/calcite/rex/RexUtil.java @@ -2607,7 +2607,11 @@ public class RexUtil { } } - /** Deep expressions simplifier. */ + /** Deep expressions simplifier. + * + * <p>This class is broken because it does not change the value of + * {@link RexUnknownAs} as it recurses into an expression. Do not use. */ + @Deprecated // to be removed before 2.0 public static class ExprSimplifier extends RexShuttle { private final RexSimplify simplify; private final Map<RexNode, RexUnknownAs> unknownAsMap = @@ -2615,12 +2619,10 @@ public class RexUtil { private final RexUnknownAs unknownAs; private final boolean matchNullability; - @Deprecated // to be removed before 2.0 public ExprSimplifier(RexSimplify simplify) { this(simplify, RexUnknownAs.UNKNOWN, true); } - @Deprecated // to be removed before 2.0 public ExprSimplifier(RexSimplify simplify, boolean matchNullability) { this(simplify, RexUnknownAs.UNKNOWN, matchNullability); } http://git-wip-us.apache.org/repos/asf/calcite/blob/4da9c0d9/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java b/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java index 3db0ccc..ae82438 100644 --- a/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java +++ b/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java @@ -4208,6 +4208,21 @@ public class RelOptRulesTest extends RelOptTestBase { + "case when MGR > 0 then deptno / MGR else null end > 1"; checkPlanning(program, sql); } + + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-2726";>[CALCITE-2726] + * ReduceExpressionRule may oversimplify filter conditions containing nulls</a>. + */ + @Test public void testNoOversimplificationBelowIsNull() { + HepProgram program = new HepProgramBuilder() + .addRuleInstance(ReduceExpressionsRule.FILTER_INSTANCE) + .build(); + + String sql = + "select * from emp where ( (empno=1 and mgr=1) or (empno=null and mgr=1) ) is null"; + checkPlanning(program, sql); + } + } // End RelOptRulesTest.java http://git-wip-us.apache.org/repos/asf/calcite/blob/4da9c0d9/core/src/test/java/org/apache/calcite/test/RexImplicationCheckerTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/calcite/test/RexImplicationCheckerTest.java b/core/src/test/java/org/apache/calcite/test/RexImplicationCheckerTest.java index 207a819..2b9b57f 100644 --- a/core/src/test/java/org/apache/calcite/test/RexImplicationCheckerTest.java +++ b/core/src/test/java/org/apache/calcite/test/RexImplicationCheckerTest.java @@ -34,7 +34,6 @@ import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.rex.RexNode; import org.apache.calcite.rex.RexSimplify; import org.apache.calcite.rex.RexUnknownAs; -import org.apache.calcite.rex.RexUtil; import org.apache.calcite.schema.SchemaPlus; import org.apache.calcite.schema.Schemas; import org.apache.calcite.server.CalciteServerStatement; @@ -345,14 +344,11 @@ public class RexImplicationCheckerTest { /** Test case for * <a href="https://issues.apache.org/jira/browse/CALCITE-2041";>[CALCITE-2041] * When simplifying a nullable expression, allow the result to change type to - * NOT NULL</a> and - * {@link org.apache.calcite.rex.RexUtil.ExprSimplifier#matchNullability}. */ + * NOT NULL</a> and match nullability. + * + * @see RexSimplify#simplifyPreservingType(RexNode, RexUnknownAs, boolean) */ @Test public void testSimplifyCastMatchNullability() { final Fixture f = new Fixture(); - final RexUtil.ExprSimplifier defaultSimplifier = - new RexUtil.ExprSimplifier(f.simplify, RexUnknownAs.UNKNOWN, true); - final RexUtil.ExprSimplifier nonMatchingNullabilitySimplifier = - new RexUtil.ExprSimplifier(f.simplify, RexUnknownAs.UNKNOWN, false); // The cast is nullable, while the literal is not nullable. When we simplify // it, we end up with the literal. If defaultSimplifier is used, a CAST is @@ -361,9 +357,13 @@ public class RexImplicationCheckerTest { // nonMatchingNullabilitySimplifier is used, the CAST is not added and the // simplified expression only consists of the literal. final RexNode e = f.cast(f.intRelDataType, f.literal(2014)); - assertThat(defaultSimplifier.apply(e).toString(), + assertThat( + f.simplify.simplifyPreservingType(e, RexUnknownAs.UNKNOWN, true) + .toString(), is("CAST(2014):JavaType(class java.lang.Integer)")); - assertThat(nonMatchingNullabilitySimplifier.apply(e).toString(), + assertThat( + f.simplify.simplifyPreservingType(e, RexUnknownAs.UNKNOWN, false) + .toString(), is("2014")); // In this case, the cast is not nullable. Thus, in both cases, the @@ -371,9 +371,13 @@ public class RexImplicationCheckerTest { RelDataType notNullIntRelDataType = f.typeFactory.createJavaType(int.class); final RexNode e2 = f.cast(notNullIntRelDataType, f.cast(notNullIntRelDataType, f.literal(2014))); - assertThat(defaultSimplifier.apply(e2).toString(), + assertThat( + f.simplify.simplifyPreservingType(e2, RexUnknownAs.UNKNOWN, true) + .toString(), is("2014")); - assertThat(nonMatchingNullabilitySimplifier.apply(e2).toString(), + assertThat( + f.simplify.simplifyPreservingType(e2, RexUnknownAs.UNKNOWN, false) + .toString(), is("2014")); } @@ -385,8 +389,6 @@ public class RexImplicationCheckerTest { final ImmutableList<TimeUnitRange> timeUnitRanges = ImmutableList.of(TimeUnitRange.YEAR, TimeUnitRange.MONTH); final Fixture f = new Fixture(); - final RexUtil.ExprSimplifier defaultSimplifier = - new RexUtil.ExprSimplifier(f.simplify, RexUnknownAs.UNKNOWN, true); final RexNode literalTs = f.timestampLiteral(new TimestampString("2010-10-10 00:00:00")); @@ -404,12 +406,18 @@ public class RexImplicationCheckerTest { final RexNode outerCeilCall = f.rexBuilder.makeCall( SqlStdOperatorTable.CEIL, innerCeilCall, f.rexBuilder.makeFlag(timeUnitRanges.get(j))); - final RexCall floorSimplifiedExpr = (RexCall) defaultSimplifier.apply(outerFloorCall); + final RexCall floorSimplifiedExpr = + (RexCall) f.simplify.simplifyPreservingType(outerFloorCall, + RexUnknownAs.UNKNOWN, true); assertThat(floorSimplifiedExpr.getKind(), is(SqlKind.FLOOR)); - assertThat(((RexLiteral) floorSimplifiedExpr.getOperands().get(1)).getValue().toString(), + assertThat(((RexLiteral) floorSimplifiedExpr.getOperands().get(1)) + .getValue().toString(), is(timeUnitRanges.get(j).toString())); - assertThat(floorSimplifiedExpr.getOperands().get(0).toString(), is(literalTs.toString())); - final RexCall ceilSimplifiedExpr = (RexCall) defaultSimplifier.apply(outerCeilCall); + assertThat(floorSimplifiedExpr.getOperands().get(0).toString(), + is(literalTs.toString())); + final RexCall ceilSimplifiedExpr = + (RexCall) f.simplify.simplifyPreservingType(outerCeilCall, + RexUnknownAs.UNKNOWN, true); assertThat(ceilSimplifiedExpr.getKind(), is(SqlKind.CEIL)); assertThat(((RexLiteral) ceilSimplifiedExpr.getOperands().get(1)).getValue().toString(), is(timeUnitRanges.get(j).toString())); @@ -432,9 +440,13 @@ public class RexImplicationCheckerTest { final RexNode outerCeilCall = f.rexBuilder.makeCall( SqlStdOperatorTable.CEIL, innerCeilCall, f.rexBuilder.makeFlag(timeUnitRanges.get(j))); - final RexCall floorSimplifiedExpr = (RexCall) defaultSimplifier.apply(outerFloorCall); + final RexCall floorSimplifiedExpr = + (RexCall) f.simplify.simplifyPreservingType(outerFloorCall, + RexUnknownAs.UNKNOWN, true); assertThat(floorSimplifiedExpr.toString(), is(outerFloorCall.toString())); - final RexCall ceilSimplifiedExpr = (RexCall) defaultSimplifier.apply(outerCeilCall); + final RexCall ceilSimplifiedExpr = + (RexCall) f.simplify.simplifyPreservingType(outerCeilCall, + RexUnknownAs.UNKNOWN, true); assertThat(ceilSimplifiedExpr.toString(), is(outerCeilCall.toString())); } } http://git-wip-us.apache.org/repos/asf/calcite/blob/4da9c0d9/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml ---------------------------------------------------------------------- diff --git a/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml b/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml index dd775ff..192e573 100644 --- a/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml +++ b/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml @@ -3114,6 +3114,25 @@ LogicalProject(EXPR$0=[CAST(/($2, $3)):INTEGER NOT NULL]) ]]> </Resource> </TestCase> + <TestCase name="testNoOversimplificationBelowIsNull"> + <Resource name="sql"> + <![CDATA[select * from emp where ( (empno=1 and mgr=1) or (empno=null and mgr=1) ) is null]]> + </Resource> + <Resource name="planBefore"> + <![CDATA[ +LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8]) + LogicalFilter(condition=[IS NULL(OR(AND(=($0, 1), =($3, 1)), AND(=($0, null), =($3, 1))))]) + LogicalTableScan(table=[[CATALOG, SALES, EMP]]) +]]> + </Resource> + <Resource name="planAfter"> + <![CDATA[ +LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8]) + LogicalFilter(condition=[IS NULL(OR(AND(=($0, 1), =($3, 1)), AND(null, =($3, 1))))]) + LogicalTableScan(table=[[CATALOG, SALES, EMP]]) +]]> + </Resource> + </TestCase> <TestCase name="testOrAlwaysTrue"> <Resource name="sql"> <![CDATA[select * from EMPNULLABLES_20 @@ -6855,7 +6874,7 @@ LogicalProject(NEWCOL=[+($0, CASE(=('a', 'a'), 1, null))]) </Resource> <Resource name="planAfter"> <![CDATA[ -LogicalProject(NEWCOL=[+($0, 1)]) +LogicalProject(NEWCOL=[+($0, CAST(1):INTEGER)]) LogicalTableScan(table=[[CATALOG, SALES, EMP]]) ]]> </Resource> @@ -8705,7 +8724,7 @@ LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$ <Resource name="planAfter"> <![CDATA[ LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8]) - LogicalFilter(condition=[AND(>($3, 0), CASE(>($3, 0), >(/($7, $3), 1), false))]) + LogicalFilter(condition=[AND(>($3, 0), CASE(>($3, 0), >(/($7, $3), 1), null))]) LogicalTableScan(table=[[CATALOG, SALES, EMP]]) ]]> </Resource> http://git-wip-us.apache.org/repos/asf/calcite/blob/4da9c0d9/core/src/test/resources/sql/conditions.iq ---------------------------------------------------------------------- diff --git a/core/src/test/resources/sql/conditions.iq b/core/src/test/resources/sql/conditions.iq index bc565af..70f7b76 100644 --- a/core/src/test/resources/sql/conditions.iq +++ b/core/src/test/resources/sql/conditions.iq @@ -258,4 +258,17 @@ select "value" from "nullables" a !ok +# Test case for [CALCITE-2726] based on [HIVE-20617] +with ax(s, t) as (values ('a','a'),('a','a '),('b','bb')) +select 'expected 1' as e,count(*) as c +from ax where ((s,t) in (('a','a'),(null, 'bb'))) is null; ++------------+---+ +| E | C | ++------------+---+ +| expected 1 | 1 | ++------------+---+ +(1 row) + +!ok + # End conditions.iq
