-
Notifications
You must be signed in to change notification settings - Fork 25.2k
ESQL: Push down filter passed lookup join #118410
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.
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
Conversation
Hi @costin, I've created a changelog YAML for you. |
Improve the planner to detect filters that can be pushed down 'through' a LOOKUP JOIN by determining the conditions scoped to the left/main side and moving them closer to the source. Relates elastic#118305
9fc0020
to
5010855
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I believe this approach is simple, succinct and correct. Needs a couple tests and it's good to go IMO.
AttributeSet rightOutput = right.outputSet(); | ||
|
||
// first remove things that left scoped only | ||
rest.removeIf(f -> f.references().subsetOf(leftOutput) && leftFilters.add(f)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funky use of &&
:D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
➕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM SoFar.
...ain/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineFilters.java
Outdated
Show resolved
Hide resolved
...ain/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineFilters.java
Show resolved
Hide resolved
Pinging @elastic/es-analytical-engine (Team:Analytics) |
3 | Error | ||
; | ||
|
||
lookupWithFieldOnJoinKey-Ignored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter on right side removes all data, likely due to considering the field as missing.
We have a bug for that so I just disabled the test for now.
10093 | 3 | Spanish | ||
; | ||
|
||
lookupMessageWithFilterOnRightSideField-Ignored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter on right side removes all data, likely due to considering the field as missing.
We have a bug for that so I just disabled the test for now.
// | ||
// Lookup JOIN | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to double check the pushdown happens inside the plan and since our CSV tests don't support (yet) lookup queries in non-Lucene mode, I've added added a bunch of testing her (such as the case with rename or breaking the filter up).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
assertThat(join.config().type(), equalTo(JoinTypes.LEFT)); | ||
project = as(join.left(), Project.class); | ||
var filter = as(project.child(), Filter.class); | ||
// assert that the rename has been undone and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what a cliffhanger.
// | ||
|
||
/** | ||
* Filter on join keys should be pushed donw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Filter on join keys should be pushed donw | |
* Filter on join keys should be pushed down |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have also tried another, more interesting, query which passed, if you'd like to add it to the csv-spec file. Either way LGTM.
The query is
FROM employees
| WHERE CASE(languages <= 1, 1, languages <= 2, 2, 3) >= 3
| RENAME languages AS language_code
| LOOKUP JOIN languages_lookup ON language_code
| SORT emp_no
| KEEP emp_no, language_code, language_name
| WHERE emp_no >= 10091 AND emp_no < 10094 OR language_code > 3
// split the filter condition in 3 parts: | ||
// 1. filter scoped to the left | ||
// 2. filter scoped to the right | ||
// 3. filter that requires both sides to be evaluated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I don't think this duplicated comment here is necessary, since the method itself has the same javadoc.
💚 Backport successful
|
…18702) Improve the planner to detect filters that can be pushed down 'through' a LOOKUP JOIN by determining the conditions scoped to the left/main side and moving them closer to the source. Relates elastic#118305
Improve the planner to detect filters that can be pushed down 'through'
a LOOKUP JOIN by determining the conditions scoped to the left/main
side and moving them closer to the source.
Relates #118305