Skip to content

[CALCITE-4499] FilterJoinRule misses opportunity to push filter to se… #2349

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[CALCITE-4499] FilterJoinRule misses opportunity to push filter to se… #2349

wants to merge 3 commits into from

Conversation

jcamachor
Copy link
Contributor

…mijoin input

zabetak
zabetak reviewed Feb 17, 2021
View reviewed changes
Copy link
Member

@zabetak zabetak left a comment

Choose a reason for hiding this comment

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

Overall LGTM.

Minor: maybe it would help for future reference if the JIRA summary/commit msg/test method was more specific that the change is needed in order to push things to the right side (filtering side) of the semi join.

zabetak
zabetak approved these changes Feb 21, 2021
View reviewed changes
Copy link
Member

@zabetak zabetak left a comment

Choose a reason for hiding this comment

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

I had a second look and left some minor comments but nothing blocking. +1

@rubenada
Copy link
Contributor

@jcamachor do you think you could make a final check on the PR (rebase, resolve conflits, check @zabetak last minor comments, ...) so that we can squeeze this in the next version?

@jcamachor
Copy link
Contributor Author

@rubenada , @zabetak , thanks for the reminder. Let me rebase and push it to the finish line.

@zabetak
Copy link
Member

zabetak commented Jun 2, 2021

@rubenada , @zabetak , thanks for the reminder. Let me rebase and push it to the finish line.

@jcamachor well don't push it immediately since we have a code freeze (ongoing release) :)

@jcamachor
Copy link
Contributor Author

Yeah, absolutely, I'm first updating the PR.

jcamachor added 3 commits June 2, 2021 10:47
@jcamachor
[CALCITE-4499] FilterJoinRule misses opportunity to push filter to se…
dd6c284 
…mijoin input
@jcamachor
Addendum
81898de 
- Deprecate current RelOptUtil.classifyFilters since it relies on input flags and join semantics to classify filter conditions
- Create new RelOptUtil.classifyFilters that does not rely on join type
- Create utility methods in JoinType to get whether filter conditions can be pushed from above/within into join, to its left input, or right input
- Change all calls to RelOptUtil.classifyFilters to use new method
@jcamachor
Addendum
5b8e4b6 
- Deprecated javadoc.
- Experimental annotation.
@asfgit asfgit closed this in de48e55 Jun 3, 2021
@jcamachor jcamachor deleted the CALCITE-4499 branch June 3, 2021 23:38
XuQianJin-Stars pushed a commit to XuQianJin-Stars/calcite that referenced this pull request Jul 14, 2021
@jcamachor
[CALCITE-4499] FilterJoinRule misses opportunity to push filter to se…
27c6e4e 
…mijoin input

Close apache#2349
hannerwang pushed a commit to hannerwang/calcite that referenced this pull request Aug 17, 2021
@jcamachor
[CALCITE-4499] FilterJoinRule misses opportunity to push filter to se…
48e5d13 
…mijoin input

Close apache#2349
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rubenada rubenada rubenada left review comments

@zabetak zabetak zabetak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jcamachor @rubenada @zabetak