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

            duplicates

            Bug - A problem which impairs or prevents the functions of the product. CALCITE-1650 Difference in BOOLEAN cast between Hive and Calcite

            • Major - Major loss of function.
            • Closed

            Bug - A problem which impairs or prevents the functions of the product. CALCITE-1651 Difference in DATE cast between Hive and Calcite

            • Major - Major loss of function.
            • Closed
            is related to

            Bug - A problem which impairs or prevents the functions of the product. CALCITE-1536 Initialize cluster before planner

            • Major - Major loss of function.
            • Open
            relates to

            Bug - A problem which impairs or prevents the functions of the product. CALCITE-1439 Handle errors during constant reduction

            • Major - Major loss of function.
            • Closed

            Bug - A problem which impairs or prevents the functions of the product. CALCITE-1620 CAST('<string>' AS BINARY) is supported by Hive, but not by Calcite

            • Major - Major loss of function.
            • Closed

            Bug - A problem which impairs or prevents the functions of the product. CALCITE-1650 Difference in BOOLEAN cast between Hive and Calcite

            • Major - Major loss of function.
            • Closed

            Bug - A problem which impairs or prevents the functions of the product. CALCITE-1651 Difference in DATE cast between Hive and Calcite

            • Major - Major loss of function.
            • Closed

            Bug - A problem which impairs or prevents the functions of the product. CALCITE-1695 Not all RexUtil.simplifyXxx code paths carry the provided executor

            • Major - Major loss of function.
            • Closed

              Activity

              Ascending order - Click to sort in descending order
              Hide
              Permalink
              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
              Permalink
              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
              Permalink
              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.
              Hide
              Permalink
              rusanu Remus Rusanu added a comment -

              https://github.com/apache/calcite/pull/379

              Show
              rusanu Remus Rusanu added a comment - https://github.com/apache/calcite/pull/379
              Hide
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              rusanu Remus Rusanu added a comment -

              I updated the PR

              Show
              rusanu Remus Rusanu added a comment - I updated the PR
              Hide
              Permalink
              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
              Permalink
              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
              Permalink
              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
              Permalink
              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!

              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
              Permalink
              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
              Permalink
              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