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

Add mutable equivalents for all relational expressions

    Details

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

      Description

      Missing MutableRels include:
      MutableCalc,
      MutableCollect,
      MutableCorrelate,
      MutableExchange,
      MutableIntersect,
      MutableMinus,
      MutableSample,
      MutableSemiJoin,
      MutableTableFunctionScan,
      MutableTableModify,
      MutableUncollect,
      MutableWindow

        Activity

        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).
        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=316a058 .
        Hide
        julianhyde Julian Hyde added a comment -

        I was glad to see that the mutable package does not reference the materialize package. Very clean!

        A few minor changes:

        • In MutableRel, I think you should make type, cluster and rowType public, apply Preconditions.checkNotNull in the constructor, and remove their accessor methods.
        • Move enum MutableRelType from MutableRel to top-level (package protected).
        • Add MutableRelTest to CalciteSuite.
        • Add a javadoc comment to each MutableXxx's "of" method, since that is the official constructor API.

        With those, +1.

        Show
        julianhyde Julian Hyde added a comment - I was glad to see that the mutable package does not reference the materialize package. Very clean! A few minor changes: In MutableRel, I think you should make type, cluster and rowType public, apply Preconditions.checkNotNull in the constructor, and remove their accessor methods. Move enum MutableRelType from MutableRel to top-level (package protected). Add MutableRelTest to CalciteSuite. Add a javadoc comment to each MutableXxx's "of" method, since that is the official constructor API. With those, +1.
        Hide
        maryannxue Maryann Xue added a comment -

        Could you please take a look at https://github.com/apache/calcite/pull/377? I moved all mutable rels into an independent package and added a new unit test MutableRelTest as well as a new test case in MaterializationTest.

        Show
        maryannxue Maryann Xue added a comment - Could you please take a look at https://github.com/apache/calcite/pull/377? I moved all mutable rels into an independent package and added a new unit test MutableRelTest as well as a new test case in MaterializationTest .
        Hide
        julianhyde Julian Hyde added a comment -

        It's that old chestnut: do you publish code as a re-usable API when you realize it could be general-purpose, or do you wait until you have a second user? I wouldn't wait for a second user if it the code's purpose is easy to explain and the API is straightforward.

        I think that the Mutable rels are in that category. If we move them to a separate package that has a clear and simple purpose it makes all of the code easier to understand.

        Show
        julianhyde Julian Hyde added a comment - It's that old chestnut: do you publish code as a re-usable API when you realize it could be general-purpose, or do you wait until you have a second user? I wouldn't wait for a second user if it the code's purpose is easy to explain and the API is straightforward. I think that the Mutable rels are in that category. If we move them to a separate package that has a clear and simple purpose it makes all of the code easier to understand.
        Hide
        maryannxue Maryann Xue added a comment - - edited

        The problem is that mutable rels are now only used by materialized view substitution. So it would be good to keep access to these classes as limited as possible, package or protected. The reason why I suggest moving the mutable rel classes into a separate file is that SubsitutionVisitor now looks a little chaotic to me, having both MutableRel classes and UnifyRule classes and a lot of utility methods, and will be even heavier once we add all these missing mutable rels. At this point, I think it might be good enough to keep all MutableRel classes and utility methods in one single file since each of these classes is relatively light. And another thing is that it doesn't seem necessary to have MaterializedViewSubstitutionVisitor since it simply contains a extended set of unify rules and meanwhile SubstitutionVisitor is not used or derived by any class other than MaterializedViewSubstitutionVisitor. So what I'm thinking right now is that all this stuff can be organized into two files:
        1. XXXSubstitutionVisitor which contains a set of unify rules and the methods that does materialization substitution (i.e. go)
        2. MutableRels which has all the mutable rel classes and the utility methods related to mutable rels (e.g. toMutable, fromMutable, etc)

        Show
        maryannxue Maryann Xue added a comment - - edited The problem is that mutable rels are now only used by materialized view substitution. So it would be good to keep access to these classes as limited as possible, package or protected. The reason why I suggest moving the mutable rel classes into a separate file is that SubsitutionVisitor now looks a little chaotic to me, having both MutableRel classes and UnifyRule classes and a lot of utility methods, and will be even heavier once we add all these missing mutable rels. At this point, I think it might be good enough to keep all MutableRel classes and utility methods in one single file since each of these classes is relatively light. And another thing is that it doesn't seem necessary to have MaterializedViewSubstitutionVisitor since it simply contains a extended set of unify rules and meanwhile SubstitutionVisitor is not used or derived by any class other than MaterializedViewSubstitutionVisitor. So what I'm thinking right now is that all this stuff can be organized into two files: 1. XXXSubstitutionVisitor which contains a set of unify rules and the methods that does materialization substitution (i.e. go ) 2. MutableRels which has all the mutable rel classes and the utility methods related to mutable rels (e.g. toMutable , fromMutable , etc)
        Hide
        julianhyde Julian Hyde added a comment -

        Maybe. Or should we go even further and make a new package org.apache.calcite.rel.mutable? (And therefore the MutableXxx classes would be public.) The question is: how much functionality in the mutable rels is specific to materialized view matching? If little or none, a package makes sense.

        Show
        julianhyde Julian Hyde added a comment - Maybe. Or should we go even further and make a new package org.apache.calcite.rel.mutable? (And therefore the MutableXxx classes would be public.) The question is: how much functionality in the mutable rels is specific to materialized view matching? If little or none, a package makes sense.
        Hide
        maryannxue Maryann Xue added a comment -

        Julian Hyde, do you think it would be reasonable to pull out all the MutableRel classes into a new file and make it package access?

        Show
        maryannxue Maryann Xue added a comment - Julian Hyde , do you think it would be reasonable to pull out all the MutableRel classes into a new file and make it package access?

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development