[CALCITE-3979] Simplification might have removed CAST expression(s) i…

…ncorrectly ReduceExpressionsRule might have removed cast expressions and inadvertly changed the type of some subexpressions. In case the rewriting have at some point arrived at: CAST(CAST(X,T),T) AND <(CAST(X,T),N)
apache · May 14, 2020 · ffa22f7 · ffa22f7
1 parent 2e30293
commit ffa22f7
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 79 deletions.
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
@@ -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<RexNode> constExps = new ArrayList<>();
     List<Boolean> addCasts = new ArrayList<>();
-    final List<RexNode> 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<RexNode> 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<RexNode> 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<RexNode> exps, ImmutableMap<RexNode, RexNode> constants,
-      List<RexNode> constExps, List<Boolean> addCasts,
-      List<RexNode> removableCasts) {
+      List<RexNode> constExps, List<Boolean> 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<Boolean> addCasts;
 
-    private final List<RexNode> removableCasts;
-
     private final Deque<SqlOperator> parentCallTypeStack = new ArrayDeque<>();
 
     ReducibleExprLocator(RelDataTypeFactory typeFactory,
         ImmutableMap<RexNode, RexNode> constants, List<RexNode> constExprs,
-        List<Boolean> addCasts, List<RexNode> removableCasts) {
+        List<Boolean> 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<RexNode> 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
@@ -557,6 +557,23 @@ protected DiffRepository getDiffRepos() {
     SqlToRelTestBase.assertValid(output);
   }
 
+  /** Test case for
+   * <a href="https://issues.apache.org/jira/browse/CALCITE-3979">[CALCITE-3979]
+   * ReduceExpressionsRule might have removed CAST expression(s) incorrectly</a>. */
+  @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
    * <a href="https://issues.apache.org/jira/browse/CALCITE-3887">[CALCITE-3887]
    * Filter and Join conditions may not need to retain nullability during simplifications</a>. */

diff --git 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]])
+]]>
+        </Resource>
+    </TestCase>
+    <TestCase name="testCastRemove">
+        <Resource name="sql">
+            <![CDATA[select
+case when cast(ename as double) < 5 then 0.0
+     else coalesce(cast(ename as double), 1.0)
+     end as t
+ from (
+       select
+          case when ename > 'abc' then ename
+               else null
+               end as ename from emp
+ )]]>
+        </Resource>
+        <Resource name="planBefore">
+            <![CDATA[
+LogicalProject(T=[CASE(<(CAST(CASE(>($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]])
+]]>
+        </Resource>
+        <Resource name="planAfter">
+            <![CDATA[
+LogicalProject(T=[CASE(<(CAST(CASE(>($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]])
 ]]>
         </Resource>
     </TestCase>
@@ -12485,7 +12511,9 @@ LogicalProject(**=[$1])
     </TestCase>
     <TestCase name="testSwapOuterJoinFieldAccess">
         <Resource name="sql">
-            <![CDATA[select t1.name, e.ename from DEPT_NESTED as t1 left outer join sales.emp e on t1.skill.type = e.job]]>
+            <![CDATA[select t1.name, e.ename
+from DEPT_NESTED as t1 left outer join sales.emp e
+ on t1.skill.type = e.job]]>
         </Resource>
         <Resource name="planBefore">
             <![CDATA[