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

Pass an expression executor to RexUtil.simplify for 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

      Allow for custom RelOptPlanner.Executor in RexUtil.simplify. RexUtil.simplify (and derivatives) use a default EXECUTOR for evaluating literals and expression in simplifyCast. This causes value differences from Hive, triggering issues like CALCITE-1650, CALCITE-1651.

        Issue Links

          Activity

          Hide
          rusanu Remus Rusanu added a comment - - edited

          In some cases the executor has to percolate through quite a lengthy stack to reach the place is needed:

          Thread [00a86df3-c98c-4766-b3ee-3ab82a63219e main] (Suspended (exception NumberFormatException))	
          	BigDecimal.<init>(double, MathContext) line: 895	
          	RexBuilder.clean(Object, RelDataType) line: 1390	
          	RexBuilder.makeLiteral(Object, RelDataType, boolean) line: 1237	
          	RexBuilder.makeLiteral(Object, RelDataType, boolean) line: 1234	
          	RexExecutable.reduce(RexBuilder, List<RexNode>, List<RexNode>) line: 84	
          	RexExecutorImpl.reduce(RexBuilder, List<RexNode>, List<RexNode>) line: 129	
          	RexUtil.simplifyCast(RexBuilder, RexCall, RelOptPlanner$Executor) line: 2387	   <-- we need the executor here
          	RexUtil.simplify(RexBuilder, RexNode, boolean, RelOptPlanner$Executor) line: 1626	
          	RexUtil$ExprSimplifier.visitCall(RexCall) line: 2984	
          	RexUtil$ExprSimplifier.visitCall(RexCall) line: 2953	
          	RexCall.accept(RexVisitor<R>) line: 107	
          	RexUtil$ExprSimplifier(RexShuttle).apply(RexNode) line: 275	
          	ReduceExpressionsRule.reduceExpressions(RelNode, List<RexNode>, RelOptPredicateList, boolean) line: 468	
          	ReduceExpressionsRule.reduceExpressions(RelNode, List<RexNode>, RelOptPredicateList) line: 445	
          	ReduceExpressionsRule$ProjectReduceExpressionsRule.onMatch(RelOptRuleCall) line: 268	
          	HepPlanner(AbstractRelOptPlanner).fireRule(RelOptRuleCall) line: 316	
          	HepPlanner.applyRule(RelOptRule, HepRelVertex, boolean) line: 506	
          	HepPlanner.applyRules(Collection<RelOptRule>, boolean) line: 385	
          	HepPlanner.executeInstruction(HepInstruction$RuleCollection) line: 279	
          	HepInstruction$RuleCollection.execute(HepPlanner) line: 72	
          	HepPlanner.executeProgram(HepProgram) line: 210	
          	HepPlanner.findBestExp() line: 197	
          	CalcitePlanner$CalcitePlannerAction.hepPlan(RelNode, boolean, RelMetadataProvider, RelOptPlanner$Executor, HepMatchOrder, RelOptRule...) line: 1749	  <-- we have the executor here
          
          Show
          rusanu Remus Rusanu added a comment - - edited In some cases the executor has to percolate through quite a lengthy stack to reach the place is needed: Thread [00a86df3-c98c-4766-b3ee-3ab82a63219e main] (Suspended (exception NumberFormatException)) BigDecimal.<init>(double, MathContext) line: 895 RexBuilder.clean(Object, RelDataType) line: 1390 RexBuilder.makeLiteral(Object, RelDataType, boolean) line: 1237 RexBuilder.makeLiteral(Object, RelDataType, boolean) line: 1234 RexExecutable.reduce(RexBuilder, List<RexNode>, List<RexNode>) line: 84 RexExecutorImpl.reduce(RexBuilder, List<RexNode>, List<RexNode>) line: 129 RexUtil.simplifyCast(RexBuilder, RexCall, RelOptPlanner$Executor) line: 2387 <-- we need the executor here RexUtil.simplify(RexBuilder, RexNode, boolean, RelOptPlanner$Executor) line: 1626 RexUtil$ExprSimplifier.visitCall(RexCall) line: 2984 RexUtil$ExprSimplifier.visitCall(RexCall) line: 2953 RexCall.accept(RexVisitor<R>) line: 107 RexUtil$ExprSimplifier(RexShuttle).apply(RexNode) line: 275 ReduceExpressionsRule.reduceExpressions(RelNode, List<RexNode>, RelOptPredicateList, boolean) line: 468 ReduceExpressionsRule.reduceExpressions(RelNode, List<RexNode>, RelOptPredicateList) line: 445 ReduceExpressionsRule$ProjectReduceExpressionsRule.onMatch(RelOptRuleCall) line: 268 HepPlanner(AbstractRelOptPlanner).fireRule(RelOptRuleCall) line: 316 HepPlanner.applyRule(RelOptRule, HepRelVertex, boolean) line: 506 HepPlanner.applyRules(Collection<RelOptRule>, boolean) line: 385 HepPlanner.executeInstruction(HepInstruction$RuleCollection) line: 279 HepInstruction$RuleCollection.execute(HepPlanner) line: 72 HepPlanner.executeProgram(HepProgram) line: 210 HepPlanner.findBestExp() line: 197 CalcitePlanner$CalcitePlannerAction.hepPlan(RelNode, boolean, RelMetadataProvider, RelOptPlanner$Executor, HepMatchOrder, RelOptRule...) line: 1749 <-- we have the executor here
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Simplifying cast on constant was a good idea, but to remain as extensible as we were before, I think we should rely on the planner associated Executor if it is available, instead of using default Calcite executor. Julian Hyde, what do you think? This should be an easy fix or am I missing something?

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Simplifying cast on constant was a good idea, but to remain as extensible as we were before, I think we should rely on the planner associated Executor if it is available, instead of using default Calcite executor. Julian Hyde , what do you think? This should be an easy fix or am I missing something?
          Hide
          rusanu Remus Rusanu added a comment -

          Turns out traversing that stack is not hard at all. ReduceExpressionsRule.reduceExpressions can get the executor from the RelNode and then extending RexUtil$ExprSimplifier to take an executor is trivial. I'm testing it now and it looks promising.

          Show
          rusanu Remus Rusanu added a comment - Turns out traversing that stack is not hard at all. ReduceExpressionsRule.reduceExpressions can get the executor from the RelNode and then extending RexUtil$ExprSimplifier to take an executor is trivial. I'm testing it now and it looks promising.
          Show
          rusanu Remus Rusanu added a comment - https://github.com/apache/calcite/pull/379
          Hide
          julianhyde Julian Hyde added a comment -

          The change looks basically good. Can you please make the executor mandatory? (The method overload without the executor would supply the default executor.)

          Also can you please rebase onto latest master branch, squash, and remove merge commits.

          We may have to revisit this (and break things) when we fix CALCITE-1536. There might no longer be a link from RelNode to RelOptPlanner; executor could become a property of the cluster.

          Show
          julianhyde Julian Hyde added a comment - The change looks basically good. Can you please make the executor mandatory? (The method overload without the executor would supply the default executor.) Also can you please rebase onto latest master branch, squash, and remove merge commits. We may have to revisit this (and break things) when we fix CALCITE-1536 . There might no longer be a link from RelNode to RelOptPlanner; executor could become a property of the cluster.
          Hide
          julianhyde Julian Hyde added a comment -

          Jesus Camacho Rodriguez, See my comments on CALCITE-1417 about why we can't use the planner's executor. Basically, there are life-cycle issues; there might not be a planner available yet. CALCITE-1536 will improve life cycle issues but we still might decide to not use the (expensive) full executor at every point in the query preparation process. For instance, at sql-to-rel time, we might use a simple executor that refuses to simplify CAST(string AS DATE).

          Show
          julianhyde Julian Hyde added a comment - Jesus Camacho Rodriguez , See my comments on CALCITE-1417 about why we can't use the planner's executor. Basically, there are life-cycle issues; there might not be a planner available yet. CALCITE-1536 will improve life cycle issues but we still might decide to not use the (expensive) full executor at every point in the query preparation process. For instance, at sql-to-rel time, we might use a simple executor that refuses to simplify CAST(string AS DATE) .
          Hide
          julianhyde Julian Hyde added a comment -

          Remus Rusanu, One more thing. The rex package should not depend on the plan package (except for RelOptUtils). So, can you rename interface RelOptPlanner.Executor to org.apache.calcite.rex.RexExecutor. If you think it will help compatibility, deprecate RelOptPlanner.Executor and have it as a base class for the new RexExecutor.

          Show
          julianhyde Julian Hyde added a comment - Remus Rusanu , One more thing. The rex package should not depend on the plan package (except for RelOptUtils). So, can you rename interface RelOptPlanner.Executor to org.apache.calcite.rex.RexExecutor . If you think it will help compatibility, deprecate RelOptPlanner.Executor and have it as a base class for the new RexExecutor .
          Hide
          rusanu Remus Rusanu added a comment -

          I updated the PR

          Show
          rusanu Remus Rusanu added a comment - I updated the PR
          Hide
          rusanu Remus Rusanu added a comment -

          I had to leave in the test for null executor because there are several callers that pass in an executor but it may be null, eg. RelBuilder.project.

          Show
          rusanu Remus Rusanu added a comment - I had to leave in the test for null executor because there are several callers that pass in an executor but it may be null, eg. RelBuilder.project .
          Hide
          julianhyde Julian Hyde added a comment -

          I solved the problem of the null executor by adding executor as a field of RelBuilder. It is populated first from the context (same place as RelBuilder finds its various RelNode factories), second from the RelNode's planner, and if these are both null uses the default executor RexUtil.EXECUTOR. So, we can safely assert that executor is never null in simplify.

          I am testing now, will check in shortly.

          Show
          julianhyde Julian Hyde added a comment - I solved the problem of the null executor by adding executor as a field of RelBuilder . It is populated first from the context (same place as RelBuilder finds its various RelNode factories), second from the RelNode 's planner, and if these are both null uses the default executor RexUtil.EXECUTOR . So, we can safely assert that executor is never null in simplify . I am testing now, will check in shortly.
          Hide
          rusanu Remus Rusanu added a comment -

          If you check in, can you please push new 12-SNAPSHOT to Maven, please? This will allow me to submit a new HIVE-15708 infra run.

          Show
          rusanu Remus Rusanu added a comment - If you check in, can you please push new 12-SNAPSHOT to Maven, please? This will allow me to submit a new HIVE-15708 infra run.
          Hide
          julianhyde Julian Hyde added a comment -
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/88d1d67d . Thanks for the PR, Remus Rusanu !
          Hide
          julianhyde Julian Hyde added a comment -

          Remus Rusanu, OK, I've pushed a snapshot.

          Show
          julianhyde Julian Hyde added a comment - Remus Rusanu , OK, I've pushed a snapshot.
          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