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

Use RelBuilder in rules rather than type-specific RelNode factories

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.5.0
    • Component/s: None
    • Labels:
      None

      Description

      Many planner rules create filters, projects, joins, ... and over the years people have made them generic, so they can create different particular sub-classes of RelNodes (say DrillFilter or HiveProject) by adding factories (interfaces defined in RelFactories).

      But then people extend a rule, so that a rule that used to only create projects now might sometimes create a filter. They now need to add an extra parameter to the instance of the rule, which is painful. Also, rules may call into utility methods such as RelOptUtil.pushDownJoinConditions (see CALCITE-826), which have similar problems.

      The solution is to get rid of factories in public interfaces and instead pass around a RelBuilder. A particular instance of RelBuilder has a factory inside it for each type of RelNode. In general a particular client of Calcite (e.g. Hive) will be able to use the same RelBuilder throughout a query.

      Rules are static, so a rule instance cannot contain a RelBuilder, only a ProtoRelBuilder, which can create a RelBuilder given a RelOptCluster and RelOptSchema.

        Issue Links

          Activity

          Show
          julianhyde Julian Hyde added a comment - Started work in https://github.com/julianhyde/incubator-calcite/commits/828-rule-builder .
          Hide
          julianhyde Julian Hyde added a comment -

          From an email thread:

          This will be a big change to teams such as Hive and Drill. Now would be a good time to chime in.

          The plan is that every rule instance has a ProtoRelBuilder field from which a RelBuilder can be created to be used in an onMatch method. The RelBuilder contains factories for every kind of RelNode, so we won't
          have to keep plumbing new factories in to existing rules.

          There will be other advantages. RelBuilder is an easier and more concise way of creating relational expressions. It does some useful stuff for free, like flattening ANDs and eliminating identity projections. I'm seeing the volume of code decrease and a few minor plan improvements.

          I haven't yet found a good way to deal with the pattern where if, say, ProjectFactory is null the rule is to use Project.copy to create a Project of the same type.

          I'm keeping & deprecating the old rule constructors that take a variety of XxxFactory arguments.

          As always, we try to keep API compatibility and mark the old API

          @Deprecated // to be removed before 2.0

          but the deprecated APIs are no longer tested and are likely to be flaky. We strongly suggest that you fix calls to deprecated APIs as soon as you upgrade to a more recent version of Calcite.

          Show
          julianhyde Julian Hyde added a comment - From an email thread : This will be a big change to teams such as Hive and Drill. Now would be a good time to chime in. The plan is that every rule instance has a ProtoRelBuilder field from which a RelBuilder can be created to be used in an onMatch method. The RelBuilder contains factories for every kind of RelNode, so we won't have to keep plumbing new factories in to existing rules. There will be other advantages. RelBuilder is an easier and more concise way of creating relational expressions. It does some useful stuff for free, like flattening ANDs and eliminating identity projections. I'm seeing the volume of code decrease and a few minor plan improvements. I haven't yet found a good way to deal with the pattern where if, say, ProjectFactory is null the rule is to use Project.copy to create a Project of the same type. I'm keeping & deprecating the old rule constructors that take a variety of XxxFactory arguments. As always, we try to keep API compatibility and mark the old API @Deprecated // to be removed before 2.0 but the deprecated APIs are no longer tested and are likely to be flaky. We strongly suggest that you fix calls to deprecated APIs as soon as you upgrade to a more recent version of Calcite.
          Hide
          julianhyde Julian Hyde added a comment -

          I have completed work in the branch. Can someone please review?

          Show
          julianhyde Julian Hyde added a comment - I have completed work in the branch. Can someone please review?
          Hide
          julianhyde Julian Hyde added a comment -

          Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/815fa262. Note that there are a few breaking changes in that commit.

          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/815fa262 . Note that there are a few breaking changes in that commit.
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Resolved in release 1.5.0 (2015-11-10)

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.5.0 (2015-11-10)

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              julianhyde Julian Hyde
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development