Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-3368

PLUS, MINUS and TIMES should be unsafe when simplifying ‘expression IS NULL’

    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 1.21.0
    • Fix Version/s: None
    • Component/s: core

      Description

      'is null' expression in SQL may be optimized incorrectly in the underlying implementation.

       

      When I write a Fink SQL to test overflow just like 

      select 
         case when (f0 + f1) is null then 'null' else 'not null' end
      from testTable
      

      , I found expression '(f0 + f1) is null ' has been optimized by Calcite, and the optimization may be incorrect.

       

      The underlying implementation is that Calcite's simplification logic of isNull expression in SQL will convert  from

      "f(operand0, operand1) IS NULL" to 

      "operand0 IS NULL OR operand1 IS NULL"  if the Policy of  RexNode‘s SqlKind is ANY。

      This simplification  leads to the  expression will not calculate  the real value of  f(operand0, operand1) (eg.. '(f0 + f1)' in my case ),but  '(f0 + f1)' maybe overflows after operation. 

      //org.apache.calcite.rex.RexSimplify.java
      
      private RexNode simplifyIsNull(RexNode a) {
       // Simplify the argument first,
       // call ourselves recursively to see whether we can make more progress.
       // For example, given
       // "(CASE WHEN FALSE THEN 1 ELSE 2) IS NULL" we first simplify the
       // argument to "2", and only then we can simplify "2 IS NULL" to "FALSE".
       a = simplify(a, UNKNOWN);
       if (!a.getType().isNullable() && isSafeExpression(a)) {
       return rexBuilder.makeLiteral(false);
       }
       if (RexUtil.isNull(a)) {
       return rexBuilder.makeLiteral(true);
       }
       if (a.getKind() == SqlKind.CAST) {
       return null;
       }
       switch (Strong.policy(a.getKind())) {
       case NOT_NULL:
       return rexBuilder.makeLiteral(false);
       case ANY:
       // "f" is a strong operator, so "f(operand0, operand1) IS NULL" simplifies
       // to "operand0 IS NULL OR operand1 IS NULL"
       final List<RexNode> operands = new ArrayList<>();
       for (RexNode operand : ((RexCall) a).getOperands()) {
       final RexNode simplified = simplifyIsNull(operand);
       if (simplified == null) {
       operands.add(
       rexBuilder.makeCall(SqlStdOperatorTable.IS_NULL, operand));
       } else {
       operands.add(simplified);
       }
       }
       return RexUtil.composeDisjunction(rexBuilder, operands, false);
       case AS_IS:
       default:
       return null;
       }
      }

      And most of calculating SqlKinds are assigned Policy.ANY  at present. 

      //org.apache.calcite.plan.Strong.java
      public static Policy policy(SqlKind kind) {
        return MAP.getOrDefault(kind, Policy.AS_IS);
      }
      
      ....
      
      map.put(SqlKind.PLUS, Policy.ANY);
      map.put(SqlKind.PLUS_PREFIX, Policy.ANY);
      map.put(SqlKind.MINUS, Policy.ANY);
      map.put(SqlKind.MINUS_PREFIX, Policy.ANY);
      map.put(SqlKind.TIMES, Policy.ANY);
      map.put(SqlKind.DIVIDE, Policy.ANY);
      
       * that operator evaluates to null. */
      public enum Policy {
        /** This kind of expression is never null. No need to look at its arguments,
         * if it has any. */
        NOT_NULL,
      
        /** This kind of expression has its own particular rules about whether it
         * is null. */
        CUSTOM,
      
        /** This kind of expression is null if and only if at least one of its
         * arguments is null. */
        ANY,
      
        /** This kind of expression may be null. There is no way to rewrite. */
        AS_IS,
      }

       

      It may be an obvious nonequivalent simplification in SQL. And this issue come from Flink (FLINK-14030).

      Danny Chen, Could you have a look at this?

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                Leonard Xu Leonard Xu
                Reporter:
                Leonard Xu Leonard Xu
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:

                  Time Tracking

                  Estimated:
                  Original Estimate - Not Specified
                  Not Specified
                  Remaining:
                  Remaining Estimate - 0h
                  0h
                  Logged:
                  Time Spent - 1h 40m
                  1h 40m