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

Use ProjectFactory in AggregateJoinTranposeRule and FilterJoinRule

    Details

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

      Description

      RelOptUtil.pushDownJoinConditions is called in FilterJoinRule; it might create Project operators, but a ProjectFactory parameter is not provided. RelOptUtil.createProject that is called in AggregateJoinTranposeRule has the same issue.

        Issue Links

          Activity

          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Julian Hyde, this is an issue that we were hitting in the Hive test run; as the Project operators were not created with a factory we were hitting the assertion in L394 in RelFieldTrimmer. I have created a pull request for the fix. Can I push it to master?

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Julian Hyde , this is an issue that we were hitting in the Hive test run; as the Project operators were not created with a factory we were hitting the assertion in L394 in RelFieldTrimmer. I have created a pull request for the fix. Can I push it to master?
          Hide
          jni Jinfeng Ni added a comment - - edited

          This seems to be introduced in CALCITE-457, when I enabled the logic of push down non-ansi join condition in FilterJoinRule.

          The LogicalProject created in FilterJoinRule did not cause problems for Drill. In Drill, we have a set of conversion rule, which will convert a LogicalProject into a DrillProject. These set of conversion rule, together with Calcite rules, including FilterJoinRule, will be passed to planner as a single ruleset. This will ensure LogicalProject created in various Calcite rules will eventually be converted to DrillProject.

          I browse the code a bit. Seems other rules, such as AggregateJoinTranposeRule, JoinCommuteRule may also create LogicalProject. If Hive uses those rules in the planner, similar problem may show up again.

          Show
          jni Jinfeng Ni added a comment - - edited This seems to be introduced in CALCITE-457 , when I enabled the logic of push down non-ansi join condition in FilterJoinRule. The LogicalProject created in FilterJoinRule did not cause problems for Drill. In Drill, we have a set of conversion rule, which will convert a LogicalProject into a DrillProject. These set of conversion rule, together with Calcite rules, including FilterJoinRule, will be passed to planner as a single ruleset. This will ensure LogicalProject created in various Calcite rules will eventually be converted to DrillProject. I browse the code a bit. Seems other rules, such as AggregateJoinTranposeRule, JoinCommuteRule may also create LogicalProject. If Hive uses those rules in the planner, similar problem may show up again.
          Hide
          julianhyde Julian Hyde added a comment -

          Jesus Camacho Rodriguez, The code looks good, but I get a bunch of StackOverflowErrors during the test suite:

                 at org.apache.calcite.rel.core.RelFactories$ProjectFactoryImpl.createProject(RelFactories.java:116)
                  at org.apache.calcite.plan.RelOptUtil.createProject(RelOptUtil.java:2741)
                  at org.apache.calcite.plan.RelOptUtil.createProject(RelOptUtil.java:2693)
                  at org.apache.calcite.plan.RelOptUtil.createProject(RelOptUtil.java:2621)
                  at org.apache.calcite.rel.core.RelFactories$ProjectFactoryImpl.createProject(RelFactories.java:116)
                  at org.apache.calcite.plan.RelOptUtil.createProject(RelOptUtil.java:2741)
                  at org.apache.calcite.plan.RelOptUtil.createProject(RelOptUtil.java:2693)
                  at org.apache.calcite.plan.RelOptUtil.createProject(RelOptUtil.java:2621)
          

          JdbcTest.testAggregateEmpty is one example.

          Show
          julianhyde Julian Hyde added a comment - Jesus Camacho Rodriguez , The code looks good, but I get a bunch of StackOverflowErrors during the test suite: at org.apache.calcite.rel.core.RelFactories$ProjectFactoryImpl.createProject(RelFactories.java:116) at org.apache.calcite.plan.RelOptUtil.createProject(RelOptUtil.java:2741) at org.apache.calcite.plan.RelOptUtil.createProject(RelOptUtil.java:2693) at org.apache.calcite.plan.RelOptUtil.createProject(RelOptUtil.java:2621) at org.apache.calcite.rel.core.RelFactories$ProjectFactoryImpl.createProject(RelFactories.java:116) at org.apache.calcite.plan.RelOptUtil.createProject(RelOptUtil.java:2741) at org.apache.calcite.plan.RelOptUtil.createProject(RelOptUtil.java:2693) at org.apache.calcite.plan.RelOptUtil.createProject(RelOptUtil.java:2621) JdbcTest.testAggregateEmpty is one example.
          Hide
          julianhyde Julian Hyde added a comment -

          By the way, in the next release I would like to fix CALCITE-828. It will solve a lot of problems like this one.

          Show
          julianhyde Julian Hyde added a comment - By the way, in the next release I would like to fix CALCITE-828 . It will solve a lot of problems like this one.
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - - edited

          Jinfeng Ni, thanks for the comment. As Julian Hyde suggests, I think CALCITE-828 should be the way to go forward; hopefully it will go in the next release.

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - - edited Jinfeng Ni , thanks for the comment. As Julian Hyde suggests, I think CALCITE-828 should be the way to go forward; hopefully it will go in the next release.
          Hide
          jni Jinfeng Ni added a comment -

          Right, CALCITE-828 seems to be the right way to fix all similar problems like this one.

          Show
          jni Jinfeng Ni added a comment - Right, CALCITE-828 seems to be the right way to fix all similar problems like this one.
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/0357573f .
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Thanks Julian Hyde!

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Thanks Julian Hyde !

            People

            • Assignee:
              jcamachorodriguez Jesus Camacho Rodriguez
              Reporter:
              jcamachorodriguez Jesus Camacho Rodriguez
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development