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

Make ReduceExpressionsRule extensible to transform false filter in schema-on-read case

    Details

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

      Description

      This comes from a commit in Drill's forked Calcite (https://github.com/dremio/calcite/commit/a29f007f4f4017368b6f41bf8c3aba4490a1cee8).

      ReduceExpressionRule will transform a false filter into VALUES with empty input. This only works in the case of schema-aware scenario. In schema-on-read scenario like Drill, VALUES would not provide the schema type at the planning time; schema has to be propagate during execution time.

      We should allow ReduceExpressionRule extensible so that different behavior would be allowed to handle false filter in both schema-aware case and schema-on-read case.

        Issue Links

          Activity

          Hide
          jni Jinfeng Ni added a comment - - edited

          Jason Altekruse, could you please submit a PR to Calcite after rebasing? There might be some conflicts with CALCITE-909, which made changes to ReduceExpressionRules in Calcite 1.5.0.

          Show
          jni Jinfeng Ni added a comment - - edited Jason Altekruse , could you please submit a PR to Calcite after rebasing? There might be some conflicts with CALCITE-909 , which made changes to ReduceExpressionRules in Calcite 1.5.0.
          Hide
          julianhyde Julian Hyde added a comment -

          One approach would be to supply a Predicate<RelNode> that becomes part of the RelOptRuleOperand created in the rule constructor.

          Show
          julianhyde Julian Hyde added a comment - One approach would be to supply a Predicate<RelNode> that becomes part of the RelOptRuleOperand created in the rule constructor.
          Hide
          jaltekruse Jason Altekruse added a comment - - edited

          Julian Hyde As I understand what you are saying, this would prevent the rule from being run in the cases where the current functionality is not desired, as is the case with Drill's late types.

          What I was trying to do with this change was reuse most of the rule and just allow customizing the behavior where it is swapping out a subtree for a values operator. We could just copy the rule in Drill and give it the behavior that we want and not include the default calcite one.

          Many of the changes in the patch have already been made in master (making the classes not anonymous caused much of the diff ugliness). I have pushed a branch with the small change that I actually need to make for our existing strategy to work. Let me know if this seems reasonable.

          Show
          jaltekruse Jason Altekruse added a comment - - edited Julian Hyde As I understand what you are saying, this would prevent the rule from being run in the cases where the current functionality is not desired, as is the case with Drill's late types. What I was trying to do with this change was reuse most of the rule and just allow customizing the behavior where it is swapping out a subtree for a values operator. We could just copy the rule in Drill and give it the behavior that we want and not include the default calcite one. Many of the changes in the patch have already been made in master (making the classes not anonymous caused much of the diff ugliness). I have pushed a branch with the small change that I actually need to make for our existing strategy to work. Let me know if this seems reasonable.
          Hide
          julianhyde Julian Hyde added a comment -

          How about if the code always calls RelBuilder.values(RelDataType) but in Drill you change the behavior of the values method (by changing its ValuesFactory)?

          To do this, you would instantiate the rule using new FilterReduceExpressionsRule(LogicalFilter.class, RelBuilder.proto(DRILL_VALUES_FACTORY)).

          Show
          julianhyde Julian Hyde added a comment - How about if the code always calls RelBuilder.values(RelDataType) but in Drill you change the behavior of the values method (by changing its ValuesFactory )? To do this, you would instantiate the rule using new FilterReduceExpressionsRule(LogicalFilter.class, RelBuilder.proto(DRILL_VALUES_FACTORY)) .
          Hide
          jaltekruse Jason Altekruse added a comment - - edited

          I would like to make the solution general, but I'm not sure that this will work in this case. I do not believe the RelDataType contains enough information to produce the transformation that we want inside the context of the current rule, as it transforms the entire current section of the plan into the return of the values builder method. We want to include the entire input subtree, as we need to preserve the scan of a specific source (as well as potentially as series of other operations before the constant "false" filter, where the schema will propagate through each one).

          Show
          jaltekruse Jason Altekruse added a comment - - edited I would like to make the solution general, but I'm not sure that this will work in this case. I do not believe the RelDataType contains enough information to produce the transformation that we want inside the context of the current rule, as it transforms the entire current section of the plan into the return of the values builder method. We want to include the entire input subtree, as we need to preserve the scan of a specific source (as well as potentially as series of other operations before the constant "false" filter, where the schema will propagate through each one).
          Hide
          julianhyde Julian Hyde added a comment -

          Last try... How about builder.push(filter.getInput()).limit(0).build()), and we each change our builder so that limit(0) does something useful?

          Show
          julianhyde Julian Hyde added a comment - Last try... How about builder.push(filter.getInput()).limit(0).build()) , and we each change our builder so that limit(0) does something useful?
          Hide
          julianhyde Julian Hyde added a comment -

          Jason Altekruse, I merged your patch and then revised so that it calls builder.empty(). Please review: https://github.com/julianhyde/calcite/commit/36ca8bcdc522f8e32c2310388d5eaf381dd594a1

          If Drill overrides the createEmptyRelOrEquivalent methods it will work, but at some point you I recommend that Drill overrides the RelBuilder.empty method. Then all of the rules that create empty relational expressions will be coming through the same point, and it will be doing the right thing for Drill. And you can then remove your createEmptyRelOrEquivalent overrides.

          Show
          julianhyde Julian Hyde added a comment - Jason Altekruse , I merged your patch and then revised so that it calls builder.empty(). Please review: https://github.com/julianhyde/calcite/commit/36ca8bcdc522f8e32c2310388d5eaf381dd594a1 If Drill overrides the createEmptyRelOrEquivalent methods it will work, but at some point you I recommend that Drill overrides the RelBuilder.empty method. Then all of the rules that create empty relational expressions will be coming through the same point, and it will be doing the right thing for Drill. And you can then remove your createEmptyRelOrEquivalent overrides.
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/898fdfc2 . Thanks for the PR, Jason Altekruse !
          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.6.0 (2016-01-22).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.6.0 (2016-01-22).

            People

            • Assignee:
              jaltekruse Jason Altekruse
              Reporter:
              jni Jinfeng Ni
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development