Skip to content

Add push inequality filter expression below join rule#12236

Merged
sopel39 merged 1 commit intotrinodb:masterfrom
starburstdata:ls/015-push-filter-expression-below-join
May 5, 2022
Merged

Add push inequality filter expression below join rule#12236
sopel39 merged 1 commit intotrinodb:masterfrom
starburstdata:ls/015-push-filter-expression-below-join

Conversation

@lukasz-stec
Copy link
Copy Markdown
Member

@lukasz-stec lukasz-stec commented May 4, 2022

Description

Add rule that pushes complex expressions (not symbol references) in the join filter to the join build side to allow for dynamic filters to be applied

The rule support inequality operators only (<,<=, >,>=). It currently does not support BETWEEN operator.

Is this change a fix, improvement, new feature, refactoring, or other?

improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

core query engine - planner

How would you describe this change to a non-technical end user or system administrator?

Improve performance of joins with inequality conditions

Related issues, pull requests, and links

Fixes #5755

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label May 4, 2022
@lukasz-stec lukasz-stec force-pushed the ls/015-push-filter-expression-below-join branch from f36a8a1 to ae746fa Compare May 4, 2022 15:22
@lukasz-stec lukasz-stec marked this pull request as ready for review May 5, 2022 06:35
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we reuse leftComparisonSymbols here ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

isComparisonAligned was used in two places so I had to add JoinNodeContext to reuse leftComparisonSymbols

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will this be quadratic complexity ?
Maybe reuse Set<Symbol> leftSymbols here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes, it could be quadratic. I refactored the code to avoid this.

@sopel39
Copy link
Copy Markdown
Member

sopel39 commented May 5, 2022

lgtm % @raunaqmorarka comments

Add rule that pushes complex expressions
(not symbol references) in the join filter
to the join build side to allow for dynamic filters
to be applied
@lukasz-stec lukasz-stec force-pushed the ls/015-push-filter-expression-below-join branch from ae746fa to f2b4cf6 Compare May 5, 2022 11:00
Copy link
Copy Markdown
Member Author

@lukasz-stec lukasz-stec left a comment

Choose a reason for hiding this comment

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

I refactored the code a little bit to avoid quadratic complexity of isComparisonAligned + avoid recalculation of joinNode left and right output symbols sets

@lukasz-stec lukasz-stec requested a review from raunaqmorarka May 5, 2022 11:36
@sopel39 sopel39 merged commit ce1230a into trinodb:master May 5, 2022
@sopel39 sopel39 mentioned this pull request May 5, 2022
@github-actions github-actions bot added this to the 380 milestone May 5, 2022
@raunaqmorarka raunaqmorarka deleted the ls/015-push-filter-expression-below-join branch July 6, 2022 20:09
/**
* Rule that transform plan like
* <pre> {@code
* filter: expr(l) > expr(r)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is potentially unsafe to pushdown if expr(r) might fail. cc @martint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Support dynamic filtering for complex inequalities

3 participants