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

Handle errors during constant reduction

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.12.0
    • Component/s: None
    • Labels:
      None

      Description

      A literal is a constant, and so a cast of a literal is also a constant, but when we do constant reduction sometimes there are errors if the cast is not valid. For example, the query

      values cast('' as integer)
      

      gives ExceptionInInitializerError and, to make matters worse, the Avatica JDBC driver does not catch the exception (because it is a java.lang.Error) and wrap it as a java.sql.SQLException as it ought to.

      Note that cast('1.2' as integer) is also invalid but cast(' -1 ' as integer) is valid.

      This issue probably also applies to divide-by-zero and other exceptions evaluating scalar expressions.

        Issue Links

          Activity

          Hide
          rusanu Remus Rusanu added a comment -

          For testing Hive with 1.12 I added catch in RexUtil.simplify and return the original RexNode if any exception occurs. This solved most of the issues I was seeing with the Hive testing, but I'm not sure is the right fix.

          Show
          rusanu Remus Rusanu added a comment - For testing Hive with 1.12 I added catch in RexUtil.simplify and return the original RexNode if any exception occurs. This solved most of the issues I was seeing with the Hive testing, but I'm not sure is the right fix.
          Hide
          julianhyde Julian Hyde added a comment - - edited

          Catching all errors is overkill. We should not catch, say NullPointerException, AssertionError or OutOfMemoryError. The only error that should be masked is if the constant reducer encounters a SQL runtime error while evaluating the user's expression.

          I think we need to refine the contract of the RelOptPlanner.Executor.reduce(RexBuilder rexBuilder, List<RexNode> constExps, List<RexNode> reducedValues) method. If any constant is not reduced correctly, the executor should write null (not a RexLiteral representing the NULL literal) into the corresponding slot in reducedValues. Or, if it chooses, into all slots.

          Show
          julianhyde Julian Hyde added a comment - - edited Catching all errors is overkill. We should not catch, say NullPointerException, AssertionError or OutOfMemoryError. The only error that should be masked is if the constant reducer encounters a SQL runtime error while evaluating the user's expression. I think we need to refine the contract of the RelOptPlanner.Executor.reduce(RexBuilder rexBuilder, List<RexNode> constExps, List<RexNode> reducedValues) method. If any constant is not reduced correctly, the executor should write null (not a RexLiteral representing the NULL literal) into the corresponding slot in reducedValues . Or, if it chooses, into all slots.
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          The only error that should be masked is if the constant reducer encounters a SQL runtime error while evaluating the user's expression.

          +1 on that. Julian Hyde, what about returning the original expression in the corresponding reducedValues slot in that case? I think that would be a better way to handle this case.

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - The only error that should be masked is if the constant reducer encounters a SQL runtime error while evaluating the user's expression. +1 on that. Julian Hyde , what about returning the original expression in the corresponding reducedValues slot in that case? I think that would be a better way to handle this case.
          Hide
          julianhyde Julian Hyde added a comment -

          Yes, let's do that. ReduceExpressionsRule compares the string representations of the expressions before and after, so it would work neatly with your proposal.

          Show
          julianhyde Julian Hyde added a comment - Yes, let's do that. ReduceExpressionsRule compares the string representations of the expressions before and after, so it would work neatly with your proposal.
          Show
          julianhyde Julian Hyde added a comment - Remus Rusanu , Jesus Camacho Rodriguez , I have a proposed fix in https://github.com/julianhyde/calcite/tree/1439-constant-reduction-errors . Please review.
          Hide
          rusanu Remus Rusanu added a comment -

          BTW, when the reduction uses the HiveRexExecutorImpl it catches all Exception. should probably make it a tad more restrictive.

          Show
          rusanu Remus Rusanu added a comment - BTW, when the reduction uses the HiveRexExecutorImpl it catches all Exception . should probably make it a tad more restrictive.
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/052f8545 .
          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.12.0 (2017-03-24).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.12.0 (2017-03-24).

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              julianhyde Julian Hyde
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development