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 ca5c0befd3b..a4c019b6d50 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 @@ -37,7 +37,6 @@ import org.apache.calcite.rel.logical.LogicalProject; import org.apache.calcite.rel.logical.LogicalWindow; import org.apache.calcite.rel.metadata.RelMetadataQuery; -import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.rex.RexBuilder; import org.apache.calcite.rex.RexCall; @@ -63,7 +62,6 @@ import org.apache.calcite.sql.SqlKind; import org.apache.calcite.sql.SqlOperator; import org.apache.calcite.sql.fun.SqlRowOperator; -import org.apache.calcite.sql.fun.SqlStdOperatorTable; import org.apache.calcite.sql.type.SqlTypeName; import org.apache.calcite.tools.RelBuilder; import org.apache.calcite.tools.RelBuilderFactory; @@ -650,31 +648,10 @@ protected static boolean reduceExpressionsInternal(RelNode rel, // Find reducible expressions. final List constExps = new ArrayList<>(); List addCasts = new ArrayList<>(); - final List removableCasts = new ArrayList<>(); findReducibleExps(rel.getCluster().getTypeFactory(), expList, - predicates.constantMap, constExps, addCasts, removableCasts); - if (constExps.isEmpty() && removableCasts.isEmpty()) { - return changed; - } - - // Remove redundant casts before reducing constant expressions. - // If the argument to the redundant cast is a reducible constant, - // reducing that argument to a constant first will result in not being - // able to locate the original cast expression. - if (!removableCasts.isEmpty()) { - final List reducedExprs = new ArrayList<>(); - for (RexNode exp : removableCasts) { - RexCall call = (RexCall) exp; - reducedExprs.add(call.getOperands().get(0)); - } - RexReplacer replacer = - new RexReplacer(simplify, unknownAs, removableCasts, reducedExprs, - Collections.nCopies(removableCasts.size(), false)); - replacer.mutate(expList); - } - + predicates.constantMap, constExps, addCasts); if (constExps.isEmpty()) { - return true; + return changed; } final List constExps2 = Lists.newArrayList(constExps); @@ -739,15 +716,13 @@ protected static boolean reduceExpressionsInternal(RelNode rel, * @param addCasts indicator for each expression that can be constant * reduced, whether a cast of the resulting reduced * expression is potentially necessary - * @param removableCasts returns the list of cast expressions where the cast */ protected static void findReducibleExps(RelDataTypeFactory typeFactory, List exps, ImmutableMap constants, - List constExps, List addCasts, - List removableCasts) { + List constExps, List addCasts) { ReducibleExprLocator gardener = new ReducibleExprLocator(typeFactory, constants, constExps, - addCasts, removableCasts); + addCasts); for (RexNode exp : exps) { gardener.analyze(exp); } @@ -920,20 +895,17 @@ enum Constancy { private final List addCasts; - private final List removableCasts; - private final Deque parentCallTypeStack = new ArrayDeque<>(); ReducibleExprLocator(RelDataTypeFactory typeFactory, ImmutableMap constants, List constExprs, - List addCasts, List removableCasts) { + List addCasts) { // go deep super(true); this.typeFactory = typeFactory; this.constants = constants; this.constExprs = constExprs; this.addCasts = addCasts; - this.removableCasts = removableCasts; } public void analyze(RexNode exp) { @@ -1071,12 +1043,6 @@ private void analyzeCall(RexCall call, Constancy callConstancy) { addResult(call.getOperands().get(iOperand)); } } - - // if this cast expression can't be reduced to a literal, - // then see if we can remove the cast - if (call.getOperator() == SqlStdOperatorTable.CAST) { - reduceCasts(call); - } } // pop operands off of the stack @@ -1089,45 +1055,6 @@ private void analyzeCall(RexCall call, Constancy callConstancy) { stack.add(callConstancy); } - private void reduceCasts(RexCall outerCast) { - List operands = outerCast.getOperands(); - if (operands.size() != 1) { - return; - } - RelDataType outerCastType = outerCast.getType(); - RelDataType operandType = operands.get(0).getType(); - if (operandType.equals(outerCastType)) { - removableCasts.add(outerCast); - return; - } - - // See if the reduction - // CAST((CAST x AS type) AS type NOT NULL) - // -> CAST(x AS type NOT NULL) - // applies. TODO jvs 15-Dec-2008: consider - // similar cases for precision changes. - if (!(operands.get(0) instanceof RexCall)) { - return; - } - RexCall innerCast = (RexCall) operands.get(0); - if (innerCast.getOperator() != SqlStdOperatorTable.CAST) { - return; - } - if (innerCast.getOperands().size() != 1) { - return; - } - RelDataType outerTypeNullable = - typeFactory.createTypeWithNullability(outerCastType, true); - RelDataType innerTypeNullable = - typeFactory.createTypeWithNullability(operandType, true); - if (outerTypeNullable != innerTypeNullable) { - return; - } - if (operandType.isNullable()) { - removableCasts.add(innerCast); - } - } - @Override public Void visitDynamicParam(RexDynamicParam dynamicParam) { return pushVariable(); } 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 4ff890227e8..37eaae076a1 100644 --- a/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java +++ b/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java @@ -557,6 +557,23 @@ protected DiffRepository getDiffRepos() { SqlToRelTestBase.assertValid(output); } + /** Test case for + * [CALCITE-3979] + * ReduceExpressionsRule might have removed CAST expression(s) incorrectly. */ + @Test public void testCastRemove() throws Exception { + final String sql = "select\n" + + "case when cast(ename as double) < 5 then 0.0\n" + + " else coalesce(cast(ename as double), 1.0)\n" + + " end as t\n" + + " from (\n" + + " select\n" + + " case when ename > 'abc' then ename\n" + + " else null\n" + + " end as ename from emp\n" + + " )"; + sql(sql).withRule(ReduceExpressionsRule.PROJECT_INSTANCE).checkUnchanged(); + } + /** Test case for * [CALCITE-3887] * Filter and Join conditions may not need to retain nullability during simplifications. */ 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 2685ee7a648..07f8b80754d 100644 --- a/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml +++ b/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml @@ -681,6 +681,32 @@ LogicalProject(EMPNO=[$0]) LogicalProject(EMPNO=[$0]) LogicalFilter(condition=[OR(AND(>($5, 1000), =($0, 1)), =($5, 1))]) LogicalTableScan(table=[[CATALOG, SALES, EMP]]) +]]> + + + + + 'abc' then ename + else null + end as ename from emp + )]]> + + + ($1, 'abc'), $1, null:VARCHAR(20))):DOUBLE, 5), 0.0:DOUBLE, CASE(IS NOT NULL(CAST(CASE(>($1, 'abc'), $1, null:VARCHAR(20))):DOUBLE), CAST(CAST(CASE(>($1, 'abc'), $1, null:VARCHAR(20))):DOUBLE):DOUBLE NOT NULL, 1.0:DOUBLE))]) + LogicalTableScan(table=[[CATALOG, SALES, EMP]]) +]]> + + + ($1, 'abc'), $1, null:VARCHAR(20))):DOUBLE, 5), 0.0:DOUBLE, CASE(IS NOT NULL(CAST(CASE(>($1, 'abc'), $1, null:VARCHAR(20))):DOUBLE), CAST(CAST(CASE(>($1, 'abc'), $1, null:VARCHAR(20))):DOUBLE):DOUBLE NOT NULL, 1.0:DOUBLE))]) + LogicalTableScan(table=[[CATALOG, SALES, EMP]]) ]]> @@ -12485,7 +12511,9 @@ LogicalProject(**=[$1]) - +