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

Not all RexUtil.simplifyXxx code paths carry the provided executor

    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

      CALCITE-1653 resolved some of the issues encountered with different semantics in Hive executor vs. Calcite default executor. When investigating a workaround for CALCITE-1690 I found that there are other code paths that can this the same issue, eg:

      	at org.apache.calcite.rex.RexBuilder.makeLiteral(RexBuilder.java:1239)
      	at org.apache.calcite.rex.RexBuilder.makeLiteral(RexBuilder.java:1236)
      	at org.apache.calcite.rex.RexExecutable.reduce(RexExecutable.java:86)
      	at org.apache.calcite.rex.RexExecutorImpl.reduce(RexExecutorImpl.java:128)
      	at org.apache.calcite.rex.RexUtil.simplifyCast(RexUtil.java:2450)
      	at org.apache.calcite.rex.RexUtil.simplify(RexUtil.java:1633)
      	at org.apache.calcite.rex.RexUtil.simplify(RexUtil.java:1587)
      	at org.apache.calcite.rex.RexUtil.simplifyList(RexUtil.java:1747)
      	at org.apache.calcite.rex.RexUtil.simplifyComparison(RexUtil.java:1658)
      	at org.apache.calcite.rex.RexUtil.simplify(RexUtil.java:1648)
      	at org.apache.calcite.rex.RexUtil$ExprSimplifier.visitCall(RexUtil.java:3051)
      	at org.apache.calcite.rex.RexUtil$ExprSimplifier.visitCall(RexUtil.java:3016)
      	at org.apache.calcite.rex.RexCall.accept(RexCall.java:104)
      	at org.apache.calcite.rex.RexShuttle.apply(RexShuttle.java:279)
      	at org.apache.calcite.rel.rules.ReduceExpressionsRule.reduceExpressions(ReduceExpressionsRule.java:473)
      

      Int he stack above neither RexUtil.simplifyComparison nor RexUtil.simplifyList accept an executor, and thus the executor info present at the RexUtil.simplify:1648 frame is lost and the default EXECUTOR is used instead. The result is incorrect for Hive.

        Issue Links

          Activity

          Show
          rusanu Remus Rusanu added a comment - https://github.com/apache/calcite/pull/402
          Hide
          rusanu Remus Rusanu added a comment - - edited

          I moved the RexExecutor as a property of the RexBuilder and removed all optional RexUtil.simplifyXxx(..., RexExecutor executor). The executor is retrieved from the build only when needed, and this way it is carried w/o loss through all the simplifyXxx methods w/o having to provide overwrites for each.

          Show
          rusanu Remus Rusanu added a comment - - edited I moved the RexExecutor as a property of the RexBuilder and removed all optional RexUtil.simplifyXxx(..., RexExecutor executor) . The executor is retrieved from the build only when needed, and this way it is carried w/o loss through all the simplifyXxx methods w/o having to provide overwrites for each.
          Hide
          julianhyde Julian Hyde added a comment -

          I don't like this fix. RexBuilder used to be immutable, and therefore thread-safe, and now it isn't.

          I'm going to create an alternative fix. I plan to move the static RexUtil.simplifyXxx methods to a new class RexSimplifier which contains a RexBuilder and a RexExecutor. The static methods will still be present, but Hive must use the RexSimplifier variants.

          Show
          julianhyde Julian Hyde added a comment - I don't like this fix. RexBuilder used to be immutable, and therefore thread-safe, and now it isn't. I'm going to create an alternative fix. I plan to move the static RexUtil.simplifyXxx methods to a new class RexSimplifier which contains a RexBuilder and a RexExecutor. The static methods will still be present, but Hive must use the RexSimplifier variants.
          Hide
          julianhyde Julian Hyde added a comment -

          Remus Rusanu, Please review my alternate fix in https://github.com/julianhyde/calcite/commit/f4f70b51abf47a0ac0048dd4ef6aaa4fcda2a695. As long as you use a RexSimplify you are guaranteed to use the executor of your choice. RexBuilder is immutable again, so we don't have to worry about threading.

          Show
          julianhyde Julian Hyde added a comment - Remus Rusanu , Please review my alternate fix in https://github.com/julianhyde/calcite/commit/f4f70b51abf47a0ac0048dd4ef6aaa4fcda2a695 . As long as you use a RexSimplify you are guaranteed to use the executor of your choice. RexBuilder is immutable again, so we don't have to worry about threading.
          Hide
          julianhyde Julian Hyde added a comment -

          Remus Rusanu, See revised fix in https://github.com/julianhyde/calcite/commit/6f01752a693c9e09b7dcf9af810bb1d301137aae. I have removed some methods that were added since the previous release, deprecated some others, to encourage people to use a RexSimplify.

          Show
          julianhyde Julian Hyde added a comment - Remus Rusanu , See revised fix in https://github.com/julianhyde/calcite/commit/6f01752a693c9e09b7dcf9af810bb1d301137aae . I have removed some methods that were added since the previous release, deprecated some others, to encourage people to use a RexSimplify.
          Hide
          rusanu Remus Rusanu added a comment -

          Thanks Julian Hyde this is working fine, I tested it with the HIVE-15708 changes

          Show
          rusanu Remus Rusanu added a comment - Thanks Julian Hyde this is working fine, I tested it with the HIVE-15708 changes
          Hide
          julianhyde Julian Hyde added a comment - - edited

          Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/ca48431e. Added class RexSimplify, providing an explicit RexExecutor for methods to simplify RexNode s. The RexUtil.simplifyXxx methods that were present in release 1.11 are still present but are deprecated.

          Show
          julianhyde Julian Hyde added a comment - - edited Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/ca48431e . Added class RexSimplify , providing an explicit RexExecutor for methods to simplify RexNode s. The RexUtil.simplifyXxx methods that were present in release 1.11 are still present but are deprecated.
          Hide
          julianhyde Julian Hyde added a comment -

          Remus Rusanu, Can you please close https://github.com/apache/calcite/pull/402. I decided to use a different fix, because a mutable RexBuilder would have caused other issues.

          Show
          julianhyde Julian Hyde added a comment - Remus Rusanu , Can you please close https://github.com/apache/calcite/pull/402 . I decided to use a different fix, because a mutable RexBuilder would have caused other issues.
          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:
              rusanu Remus Rusanu
              Reporter:
              rusanu Remus Rusanu
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development