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

Decouple materialization and lattice substitution from VolcanoPlanner

    Details

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

      Description

      VolcanoPlanner now takes the "originalRoot" as the input for materialized-view substitution, so the programs used in Prepare.optimize() will not be applied to these substituted rels. For example, a correlated subquery will be de-correlated but its equivalents with materialization substitutions will not be de-correlated. So it would be nice to have a way for the substituted rels to run specific programs too before starting volcano planning.
      The easiest solution might be using the new "root" for materialization substitution instead, but it would be based on the assumption that those "pre-processing" programs are simple ones like de-correlation and field-trimming. In order to allow a more general pre-processing program set, one that could have different optimization output for the original rel and for the materialization substituted rels, we'd better have an interface or some approach to run pre-processing programs for rels after materialization substitution.

      1. CALCITE-1500-fix.patch
        9 kB
        Maryann Xue
      2. CALCITE-1500-fix2.patch
        10 kB
        Maryann Xue

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        I fully support making the Program API (or related APIs) able to express this kind of thing. If it helps, we can change the signature of run(RelOptPlanner planner, RelNode rel, RelTraitSet requiredOutputTraits); the current arguments happened without much great thought.

        Show
        julianhyde Julian Hyde added a comment - I fully support making the Program API (or related APIs) able to express this kind of thing. If it helps, we can change the signature of run(RelOptPlanner planner, RelNode rel, RelTraitSet requiredOutputTraits) ; the current arguments happened without much great thought.
        Hide
        maryannxue Maryann Xue added a comment -

        Could you please review https://github.com/apache/calcite/pull/361, Julian Hyde? This PR addresses several issues:
        1. Pull materialization substitution logic out of VolcanoPlanner. This allows a user Program to apply customized planning logic to each materialized-view(or lattice)-substituted rel. The example Program, Programs.standard2() added in https://github.com/maryannxue/calcite/tree/calcite-1500-sub (a experiment branch, not part of the PR) shows an alternative way of achieving the same same planning result as Programs.standard() by planning each substituted rel independently and choosing the winner in the final step. Ultimately, after CALCITE-1536 is ready, the new Program will create a new planner for each rel instead of calling the "clear()" method.
        2. Bug fix for RuleQueue.clear(); add interface RelOptPlanner.getRules(). This is part of CALCITE-1499 but does not involve any change to the existing clear() logic. Like mentioned above, before completion of CALCITE-1536, clear() could be a walk-around and Phoenix can use it for now.
        3. Set the "root" (the "originalRoot") as the rel after subquery rewrite and decorrelation. New test case "MaterializationTest.testSubquery()" is to verify this change. Meanwhile this change essentially reverted CALCITE-1524, which I pointed out caused some regression: 1) RelRoot.project() projects all needed fields which causes a RowType mismatch for cases like "select c1 from t order by c2"; 2) CALCITE-1524 only changed the "originalRoot" but had no effect on the rel that's eventually used in planning (the real root), and there were no test case to illustrate its purpose or for regression check.

        Show
        maryannxue Maryann Xue added a comment - Could you please review https://github.com/apache/calcite/pull/361 , Julian Hyde ? This PR addresses several issues: 1. Pull materialization substitution logic out of VolcanoPlanner. This allows a user Program to apply customized planning logic to each materialized-view(or lattice)-substituted rel. The example Program, Programs.standard2() added in https://github.com/maryannxue/calcite/tree/calcite-1500-sub (a experiment branch, not part of the PR) shows an alternative way of achieving the same same planning result as Programs.standard() by planning each substituted rel independently and choosing the winner in the final step. Ultimately, after CALCITE-1536 is ready, the new Program will create a new planner for each rel instead of calling the "clear()" method. 2. Bug fix for RuleQueue.clear(); add interface RelOptPlanner.getRules(). This is part of CALCITE-1499 but does not involve any change to the existing clear() logic. Like mentioned above, before completion of CALCITE-1536 , clear() could be a walk-around and Phoenix can use it for now. 3. Set the "root" (the "originalRoot") as the rel after subquery rewrite and decorrelation. New test case "MaterializationTest.testSubquery()" is to verify this change. Meanwhile this change essentially reverted CALCITE-1524 , which I pointed out caused some regression: 1) RelRoot.project() projects all needed fields which causes a RowType mismatch for cases like "select c1 from t order by c2"; 2) CALCITE-1524 only changed the "originalRoot" but had no effect on the rel that's eventually used in planning (the real root), and there were no test case to illustrate its purpose or for regression check.
        Hide
        julianhyde Julian Hyde added a comment - - edited

        I'm not sure we should be baking any session state into a Program. So, for instance, we should not add List<RelOptMaterialization> materializations, List<RelOptLattice> lattices parameters to the Programs.standard(). Maybe we need to discuss changing the Program.run interface. In fact we need to complete the discussion in CALCITE-1536 and figure out which state we believe should belong to the program, the planner, the rel node tree, and which should be passed into and out of the Program.run method.

        Comments:

        • I am concerned that making getRules() and ENUMERABLE_RULES public will make it more difficult to refactor in future
        • Please use "Returns ..." rather than "Return ..." in method javadoc
        • How about renaming MaterializationOptUtil to RelOptMaterializations?
        • The Pair of Pair data structure you are passing to Hook.PROGRAM is unwieldy. Either define a struct with 3 fields, or create a Tuple3 class (analogous to Pair).
        • Please rename testSubquery to testSubQuery. I am trying to standardize on sub-query being two words.
        Show
        julianhyde Julian Hyde added a comment - - edited I'm not sure we should be baking any session state into a Program. So, for instance, we should not add List<RelOptMaterialization> materializations, List<RelOptLattice> lattices parameters to the Programs.standard() . Maybe we need to discuss changing the Program.run interface. In fact we need to complete the discussion in CALCITE-1536 and figure out which state we believe should belong to the program, the planner, the rel node tree, and which should be passed into and out of the Program.run method. Comments: I am concerned that making getRules() and ENUMERABLE_RULES public will make it more difficult to refactor in future Please use "Returns ..." rather than "Return ..." in method javadoc How about renaming MaterializationOptUtil to RelOptMaterializations? The Pair of Pair data structure you are passing to Hook.PROGRAM is unwieldy. Either define a struct with 3 fields, or create a Tuple3 class (analogous to Pair). Please rename testSubquery to testSubQuery . I am trying to standardize on sub-query being two words.
        Hide
        maryannxue Maryann Xue added a comment -

        Thank you very much for the feedback, Julian Hyde! I just added a comment on CALCITE-1536. I agree with you that we need to wait for CALCITE-1536. I was trying to get the planning workflow adjustment done for Calcite-Phoenix and was wondering if we could use "RelOptPlanner.clear()" as a walk-around, although I'm inclined to have RelOptPlanner immutable and discard "clear()" method at all. I think it's hard to draw a clear line of what should be immutable and what should not in RelOptPlanner and things may change in future. Besides, on implementation level some states are dependent on others. Anyway, l'll chime in on CALCITE-1536 and put this one off for now.

        Show
        maryannxue Maryann Xue added a comment - Thank you very much for the feedback, Julian Hyde ! I just added a comment on CALCITE-1536 . I agree with you that we need to wait for CALCITE-1536 . I was trying to get the planning workflow adjustment done for Calcite-Phoenix and was wondering if we could use "RelOptPlanner.clear()" as a walk-around, although I'm inclined to have RelOptPlanner immutable and discard "clear()" method at all. I think it's hard to draw a clear line of what should be immutable and what should not in RelOptPlanner and things may change in future. Besides, on implementation level some states are dependent on others. Anyway, l'll chime in on CALCITE-1536 and put this one off for now.
        Hide
        julianhyde Julian Hyde added a comment -

        Maryann Xue, I've replied to your comments in CALCITE-1536. Can we make progress on this issue (1500) now? I'm prepared to accept short-term messiness as long as we commit to cleaning it up.

        Show
        julianhyde Julian Hyde added a comment - Maryann Xue , I've replied to your comments in CALCITE-1536 . Can we make progress on this issue (1500) now? I'm prepared to accept short-term messiness as long as we commit to cleaning it up.
        Hide
        maryannxue Maryann Xue added a comment -

        Yes, we can. So how about I make "materializations and lattices" into run() first? We may need to add traits too later on, but let's do it in CALCITE-1536?

        Show
        maryannxue Maryann Xue added a comment - Yes, we can. So how about I make "materializations and lattices" into run() first? We may need to add traits too later on, but let's do it in CALCITE-1536 ?
        Hide
        maryannxue Maryann Xue added a comment -

        Julian Hyde, I just committed changes to the PR, based on your review:
        1. Moved List<RelOptMaterialization> materializations, List<RelOptLattice> lattices to Program.run. Not sure if we should make a new struct wrapping the parameters in Program.run, like trait-set, or even rules.
        2. Added clearing materializations and lattices in RelOptPlanner and VolcanoPlanner, since it will be more aligned with the changes made in Programs.RuleSetProgram, although I believe eventually we should either remove clear method or make clear method clear almost everything.
        3. I kept getRules for I don't think it would do more harm than existing getMaterializations and getLatticeByName. If we decide to make them immutable members in RelOptPlanner in CALCITE-1536, we will still be good to have these getter methods; and if we decide to remove them from RelOptPlanner, we'll take these getter methods away altogether.

        Show
        maryannxue Maryann Xue added a comment - Julian Hyde , I just committed changes to the PR, based on your review: 1. Moved List<RelOptMaterialization> materializations, List<RelOptLattice> lattices to Program.run . Not sure if we should make a new struct wrapping the parameters in Program.run , like trait-set, or even rules. 2. Added clearing materializations and lattices in RelOptPlanner and VolcanoPlanner , since it will be more aligned with the changes made in Programs.RuleSetProgram , although I believe eventually we should either remove clear method or make clear method clear almost everything. 3. I kept getRules for I don't think it would do more harm than existing getMaterializations and getLatticeByName . If we decide to make them immutable members in RelOptPlanner in CALCITE-1536 , we will still be good to have these getter methods; and if we decide to remove them from RelOptPlanner, we'll take these getter methods away altogether.
        Hide
        julianhyde Julian Hyde added a comment -

        Maryann Xue, I agree with all 3 of those changes. Can you do the cosmetic stuff I asked for (e.g. "Returns..."), rebase and squash (it's difficult to review right now because there's a big merge commit in the middle) and I'll review one last time.

        Show
        julianhyde Julian Hyde added a comment - Maryann Xue , I agree with all 3 of those changes. Can you do the cosmetic stuff I asked for (e.g. "Returns..."), rebase and squash (it's difficult to review right now because there's a big merge commit in the middle) and I'll review one last time.
        Hide
        maryannxue Maryann Xue added a comment -

        Thanks, Julian Hyde! I just synced the branch with latest master, and cosmetic changes you suggested was already included in the previous checkin.

        Show
        maryannxue Maryann Xue added a comment - Thanks, Julian Hyde ! I just synced the branch with latest master, and cosmetic changes you suggested was already included in the previous checkin.
        Hide
        julianhyde Julian Hyde added a comment -

        +1 go ahead and commit

        Show
        julianhyde Julian Hyde added a comment - +1 go ahead and commit
        Hide
        maryannxue Maryann Xue added a comment -
        Show
        maryannxue Maryann Xue added a comment - Thank you, Julian Hyde , for the review! Fixed in https://git1-us-west.apache.org/repos/asf?p=calcite.git;a=commit;h=972b5de .
        Hide
        julianhyde Julian Hyde added a comment -

        Maryann Xue, I am seeing a regression caused by this change. It seems to happen most often on JDK7, occasionally on JDK8. Here is the output:

        Exception in thread "main" java.lang.reflect.InvocationTargetException
        	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        	at java.lang.reflect.Method.invoke(Method.java:483)
        	at org.apache.calcite.test.QuidemTest.test(QuidemTest.java:215)
        	at org.apache.calcite.test.QuidemTest.main(QuidemTest.java:80)
        	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        	at java.lang.reflect.Method.invoke(Method.java:483)
        	at com.intellij.rt.execution.application.AppMain.main(AppMain.java:147)
        Caused by: java.lang.AssertionError: Files differ: /Users/jhyde/open1/calcite.1/core/target/surefire/sql/misc.iq /Users/jhyde/open1/calcite.1/core/target/test-classes/sql/misc.iq
        513,514c513,517
        < EnumerableCalc(expr#0..4=[{inputs}], deptno=[$t1])
        <   EnumerableTableScan(table=[[hr, emps]])
        ---
        > EnumerableJoin(condition=[true], joinType=[left])
        >   EnumerableCalc(expr#0..4=[{inputs}], deptno=[$t1])
        >     EnumerableTableScan(table=[[hr, emps]])
        >   EnumerableAggregate(group=[{}])
        >     EnumerableTableScan(table=[[hr, depts]])
        

        The rule to eliminate a semi-join (because the semi-joined relation always has one row) no longer fires. Can you take a look please?

        Show
        julianhyde Julian Hyde added a comment - Maryann Xue , I am seeing a regression caused by this change. It seems to happen most often on JDK7, occasionally on JDK8. Here is the output: Exception in thread "main" java.lang.reflect.InvocationTargetException at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:483) at org.apache.calcite.test.QuidemTest.test(QuidemTest.java:215) at org.apache.calcite.test.QuidemTest.main(QuidemTest.java:80) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:483) at com.intellij.rt.execution.application.AppMain.main(AppMain.java:147) Caused by: java.lang.AssertionError: Files differ: /Users/jhyde/open1/calcite.1/core/target/surefire/sql/misc.iq /Users/jhyde/open1/calcite.1/core/target/test-classes/sql/misc.iq 513,514c513,517 < EnumerableCalc(expr#0..4=[{inputs}], deptno=[$t1]) < EnumerableTableScan(table=[[hr, emps]]) --- > EnumerableJoin(condition=[true], joinType=[left]) > EnumerableCalc(expr#0..4=[{inputs}], deptno=[$t1]) > EnumerableTableScan(table=[[hr, emps]]) > EnumerableAggregate(group=[{}]) > EnumerableTableScan(table=[[hr, depts]]) The rule to eliminate a semi-join (because the semi-joined relation always has one row) no longer fires. Can you take a look please?
        Hide
        maryannxue Maryann Xue added a comment -

        Yes, I'll take a look right away. I verified all the tests a couple of times but with JDK8 only, so guess that's the reason why I didn't see the regression.

        Show
        maryannxue Maryann Xue added a comment - Yes, I'll take a look right away. I verified all the tests a couple of times but with JDK8 only, so guess that's the reason why I didn't see the regression.
        Hide
        maryannxue Maryann Xue added a comment -

        Julian Hyde, SemiJoinRule did not fire because the query was converted into

        LogicalJoin
          LogicalProject
            LogicalTableScan
          LogicalAggregate // empty aggregate
            LogicalTableScan
        

        after field trimming. The top-level Project has been push down and then eliminated, so the new rel could not match the required pattern of SemiJoinRule as Project (Join (RelNode, Aggregate)).
        This test case was added just after check-in of CALCITE-1524 (https://git1-us-west.apache.org/repos/asf?p=calcite.git;a=commit;h=751e2b0c6d6e19bef7f07b1e0acf1ce491b59311), which would add an extra Project, so the test case could pass. My check-in for this issue (CALCITE-1500) reverted this change (CALCITE-1524) and that's why it fails now. I couldn't find out, though, why it appeared as random failure under command-line with JDK 8.
        CALCITE-1524 caused some regression because it would change the row type of the original rel if some fields are used but not returned (e.g., select empno from emp order by deptno). So I think it might make more sense to change the SemiJoinRule here to fix the issue. What do you think, Julian? I'm attaching a patch here for the suggested change.

        Show
        maryannxue Maryann Xue added a comment - Julian Hyde , SemiJoinRule did not fire because the query was converted into LogicalJoin LogicalProject LogicalTableScan LogicalAggregate // empty aggregate LogicalTableScan after field trimming. The top-level Project has been push down and then eliminated, so the new rel could not match the required pattern of SemiJoinRule as Project (Join (RelNode, Aggregate)) . This test case was added just after check-in of CALCITE-1524 ( https://git1-us-west.apache.org/repos/asf?p=calcite.git;a=commit;h=751e2b0c6d6e19bef7f07b1e0acf1ce491b59311 ), which would add an extra Project, so the test case could pass. My check-in for this issue ( CALCITE-1500 ) reverted this change ( CALCITE-1524 ) and that's why it fails now. I couldn't find out, though, why it appeared as random failure under command-line with JDK 8. CALCITE-1524 caused some regression because it would change the row type of the original rel if some fields are used but not returned (e.g., select empno from emp order by deptno). So I think it might make more sense to change the SemiJoinRule here to fix the issue. What do you think, Julian? I'm attaching a patch here for the suggested change.
        Hide
        julianhyde Julian Hyde added a comment -

        Would a better formulation of the "empty" predicate be "aggregate.getGroupSet().isEmpty()"? It includes your case where there are zero input fields, but is a bit broader. Since there is no project telling us which fields are used, do we also need to check "aggregate.getAggCallList().isEmpty()"? This ensures that there are zero columns coming from the right-hand side of the join.

        By the way, as part of my fix for CALCITE-1583, I am adding getMinRowCount. I wonder whether that is something we could use in this rule in future.

        It's cosmetic, but rather than creating a hasProject field, I would have created two sub-classes of the rule, each with an onMatch method, calling a shared perform(RelOptRuleCall call, Project project, Join join, RelNode left, Aggregate aggregate) method.

        Show
        julianhyde Julian Hyde added a comment - Would a better formulation of the "empty" predicate be "aggregate.getGroupSet().isEmpty()"? It includes your case where there are zero input fields, but is a bit broader. Since there is no project telling us which fields are used, do we also need to check "aggregate.getAggCallList().isEmpty()"? This ensures that there are zero columns coming from the right-hand side of the join. By the way, as part of my fix for CALCITE-1583 , I am adding getMinRowCount . I wonder whether that is something we could use in this rule in future. It's cosmetic, but rather than creating a hasProject field, I would have created two sub-classes of the rule, each with an onMatch method, calling a shared perform(RelOptRuleCall call, Project project, Join join, RelNode left, Aggregate aggregate) method.
        Hide
        maryannxue Maryann Xue added a comment -

        It's cosmetic, but rather than creating a hasProject field, I would have created two sub-classes of the rule, each with an onMatch method, calling a shared perform(RelOptRuleCall call, Project project, Join join, RelNode left, Aggregate aggregate) method.

        Very nice suggestion, Julian Hyde! I was wondering if there could be a better way of using hasProject.

        Would a better formulation of the "empty" predicate be "aggregate.getGroupSet().isEmpty()"? It includes your case where there are zero input fields, but is a bit broader. Since there is no project telling us which fields are used, do we also need to check "aggregate.getAggCallList().isEmpty()"? This ensures that there are zero columns coming from the right-hand side of the join.

        Do you mean I should use aggregate.getGroupSet().isEmpty() && aggregate.getAggCallList().isEmpty() instead of aggregate.getRowType().getFieldCount()==0 ?

        Show
        maryannxue Maryann Xue added a comment - It's cosmetic, but rather than creating a hasProject field, I would have created two sub-classes of the rule, each with an onMatch method, calling a shared perform(RelOptRuleCall call, Project project, Join join, RelNode left, Aggregate aggregate) method. Very nice suggestion, Julian Hyde ! I was wondering if there could be a better way of using hasProject . Would a better formulation of the "empty" predicate be "aggregate.getGroupSet().isEmpty()"? It includes your case where there are zero input fields, but is a bit broader. Since there is no project telling us which fields are used, do we also need to check "aggregate.getAggCallList().isEmpty()"? This ensures that there are zero columns coming from the right-hand side of the join. Do you mean I should use aggregate.getGroupSet().isEmpty() && aggregate.getAggCallList().isEmpty() instead of aggregate.getRowType().getFieldCount()==0 ?
        Hide
        julianhyde Julian Hyde added a comment -

        Ah, you're right, aggregate.getRowType().getFieldCount() == 0 is better. I was confused because the parameter was called input. I thought you meant the input to the Aggregate had to have zero columns. It's worth adding a comment that such an Aggregate always produces 1 row and 0 columns; both properties are necessary for this rule to work.

        Show
        julianhyde Julian Hyde added a comment - Ah, you're right, aggregate.getRowType().getFieldCount() == 0 is better. I was confused because the parameter was called input . I thought you meant the input to the Aggregate had to have zero columns. It's worth adding a comment that such an Aggregate always produces 1 row and 0 columns; both properties are necessary for this rule to work.
        Hide
        maryannxue Maryann Xue added a comment -

        Thank you, Julian Hyde, for the nice feedback! Just formed another patch, could you please review again?

        Show
        maryannxue Maryann Xue added a comment - Thank you, Julian Hyde , for the nice feedback! Just formed another patch, could you please review again?
        Hide
        maryannxue Maryann Xue added a comment -

        Created and committed in a separate JIRA CALCITE-1628 for the regression fix.

        Show
        maryannxue Maryann Xue added a comment - Created and committed in a separate JIRA CALCITE-1628 for the regression fix.
        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:
            maryannxue Maryann Xue
            Reporter:
            maryannxue Maryann Xue
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development