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

Add ProjectRemoveRule to pre-processing program of materialization substitution

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.8.0
    • Component/s: core

      Description

      In VolcanoPlanner, we apply a simple pre-processing hep program to normalize the "target" and "query" rels before materialization substitution. Currently this program runs with two rules: FilterProjectTransposeRule and ProjectMergeRule.
      We need an extra rule ProjectRemoveRule for the Phoenix use case where a secondary index (modeled as materialized views) is defined on a view so the materialized view "queryRel" may have an identity projection introduced by this view.

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        Makes sense. A unit test would make it clearer.

        Show
        julianhyde Julian Hyde added a comment - Makes sense. A unit test would make it clearer.
        Hide
        maryannxue Maryann Xue added a comment -

        Julian Hyde, It turned out that adding that single line of change exposed a few other issues in SubstitutionVisitor:
        1. Because the Project over "select *" statement no longer exists, what used to match "ProjectToProjectUnifyRule1" now goes to "FilterToFilterUnifyRule" or "FilterToProjectUnifyRule".
        2. The FilterToFilterUnifyRule has not been covered by any test cases before and does not work correctly at all after (1).
        3. Small bug in "FilterToProjectUnifyRule": it should have checked that the new projects have full coverage for the node it's going to replace.
        4. In SubstitutionVisitor.go(), we should not remove all corresponding entries once we find a match, since there can be multiple identical rels in the query, e.g. a self-join has two identical TableScans.

        Could you please take a look at https://github.com/maryannxue/calcite/tree/calcite-1182? It works for all the test case including a newly added one, but I would like to suggest more code refinement to reduce code redundancy.

        Show
        maryannxue Maryann Xue added a comment - Julian Hyde , It turned out that adding that single line of change exposed a few other issues in SubstitutionVisitor: 1. Because the Project over "select *" statement no longer exists, what used to match "ProjectToProjectUnifyRule1" now goes to "FilterToFilterUnifyRule" or "FilterToProjectUnifyRule". 2. The FilterToFilterUnifyRule has not been covered by any test cases before and does not work correctly at all after (1). 3. Small bug in "FilterToProjectUnifyRule": it should have checked that the new projects have full coverage for the node it's going to replace. 4. In SubstitutionVisitor.go(), we should not remove all corresponding entries once we find a match, since there can be multiple identical rels in the query, e.g. a self-join has two identical TableScans. Could you please take a look at https://github.com/maryannxue/calcite/tree/calcite-1182? It works for all the test case including a newly added one, but I would like to suggest more code refinement to reduce code redundancy.
        Hide
        julianhyde Julian Hyde added a comment -

        Can you mention in the javadoc why FilterToProjectUnifyRule1 is needed in addition to FilterToProjectUnifyRule; and similarly FilterToFilterUnifyRule1. I can't see any way to reduce redundancy.

        Also, can you reduce the indentation level of the strings you added to MaterlalizationTest. These days, if I am passing multi-line string constants to functions I often put them in variables:

        final String materialize = ...;
        final String query = ...;
        checkMaterialize(materialize, query);
        

        Otherwise +1

        Show
        julianhyde Julian Hyde added a comment - Can you mention in the javadoc why FilterToProjectUnifyRule1 is needed in addition to FilterToProjectUnifyRule; and similarly FilterToFilterUnifyRule1. I can't see any way to reduce redundancy. Also, can you reduce the indentation level of the strings you added to MaterlalizationTest. These days, if I am passing multi-line string constants to functions I often put them in variables: final String materialize = ...; final String query = ...; checkMaterialize(materialize, query); Otherwise +1
        Hide
        maryannxue Maryann Xue added a comment -

        Thank you, Julian Hyde, for the comments! I will make changes accordingly. One more question: Since the Operand.isWeaker() method is only called by ProjectToProjectUnifyRule1, would it be OK that we remove this method and make ProjectToProjectUnifyRule1 call SubstitutionVisitor.isWeaker(MutableRel, MutableRel) instead, like the other two rules do?

        Show
        maryannxue Maryann Xue added a comment - Thank you, Julian Hyde , for the comments! I will make changes accordingly. One more question: Since the Operand.isWeaker() method is only called by ProjectToProjectUnifyRule1, would it be OK that we remove this method and make ProjectToProjectUnifyRule1 call SubstitutionVisitor.isWeaker(MutableRel, MutableRel) instead, like the other two rules do?
        Show
        maryannxue Maryann Xue added a comment - Fixed in https://git1-us-west.apache.org/repos/asf?p=calcite.git;a=commit;h=576c1a1eeb138726387e55680f4a1bd2aa7ed023 .
        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.8.0 (2016-06-13).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.8.0 (2016-06-13).

          People

          • Assignee:
            maryannxue Maryann Xue
            Reporter:
            maryannxue Maryann Xue
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development