Skip to content

Conversation

@Jackie-Jiang
Copy link
Contributor

Customize the following 2 rules to not push down filters/transforms for lookup JOIN:

  • CoreRules.FILTER_INTO_JOIN and CoreRules.JOIN_CONDITION_PUSH: Do not push down filter to right table, but allow pushing down to left table
  • CoreRules.PROJECT_JOIN_TRANSPOSE: Disable the rule for lookup JOIN

TODO: Allow transpose to left table in CoreRules.PROJECT_JOIN_TRANSPOSE

@Jackie-Jiang Jackie-Jiang added feature multi-stage Related to the multi-stage query engine labels Nov 23, 2024
@Jackie-Jiang Jackie-Jiang force-pushed the allow_filter_in_lookup_join branch from 3cf68fa to de41d59 Compare November 23, 2024 01:22
@Jackie-Jiang Jackie-Jiang force-pushed the allow_filter_in_lookup_join branch from de41d59 to 4652829 Compare November 23, 2024 01:45
@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2024

Codecov Report

Attention: Patch coverage is 90.74074% with 10 lines in your changes missing coverage. Please review.

Project coverage is 63.94%. Comparing base (59551e4) to head (4652829).
Report is 1385 commits behind head on master.

Files with missing lines Patch % Lines
...e/pinot/calcite/rel/rules/PinotFilterJoinRule.java 89.79% 4 Missing and 6 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #14523      +/-   ##
============================================
+ Coverage     61.75%   63.94%   +2.19%     
- Complexity      207     1569    +1362     
============================================
  Files          2436     2675     +239     
  Lines        133233   146975   +13742     
  Branches      20636    22559    +1923     
============================================
+ Hits          82274    93981   +11707     
- Misses        44911    46063    +1152     
- Partials       6048     6931     +883     
Flag Coverage Δ
custom-integration1 100.00% <ø> (+99.99%) ⬆️
integration 100.00% <ø> (+99.99%) ⬆️
integration1 100.00% <ø> (+99.99%) ⬆️
integration2 0.00% <ø> (ø)
java-11 63.91% <90.74%> (+2.20%) ⬆️
java-21 63.79% <90.74%> (+2.17%) ⬆️
skip-bytebuffers-false 63.94% <90.74%> (+2.19%) ⬆️
skip-bytebuffers-true 63.77% <90.74%> (+36.04%) ⬆️
temurin 63.94% <90.74%> (+2.19%) ⬆️
unittests 63.93% <90.74%> (+2.19%) ⬆️
unittests1 55.68% <90.74%> (+8.79%) ⬆️
unittests2 34.54% <10.18%> (+6.81%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Comment on lines +100 to +107
RelOptUtil.classifyFilters(join,
aboveFilters,
joinType.canPushIntoFromAbove(),
joinType.canPushLeftFromAbove(),
canPushRight && joinType.canPushRightFromAbove(),
joinFilters,
leftFilters,
rightFilters);
Copy link
Contributor

@gortiz gortiz Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we can contribute a patch in Calcite where FilterJoinRule calls an instance method to do this so we could just extend it.

Something like:

public class FilterJoinRule extends ... {

  boolean classifyFilters(     
      RelNode joinRel,
      List<RexNode> filters,
      JoinRelType relType,
      List<RexNode> joinFilters,
      List<RexNode> leftFilters,
      List<RexNode> rightFilters) {
    RelOptUtil.classifyFilters(join,
            aboveFilters,
            joinType.canPushIntoFromAbove(),
            joinType.canPushLeftFromAbove(),
            joinType.canPushRightFromAbove(),
            joinFilters,
            leftFilters,
            rightFilters);
  }
}

So we could simplify this class by overriding that method and adding there our check.

@Jackie-Jiang Jackie-Jiang merged commit 38abc64 into apache:master Nov 25, 2024
21 checks passed
@Jackie-Jiang Jackie-Jiang deleted the allow_filter_in_lookup_join branch November 25, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature multi-stage Related to the multi-stage query engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants