ESQL: Use a must boolean statement when pushing down to Lucene when scoring is also needed#124001
Merged
astefan merged 8 commits intoelastic:mainfrom Mar 5, 2025
Merged
ESQL: Use a must boolean statement when pushing down to Lucene when scoring is also needed#124001astefan merged 8 commits intoelastic:mainfrom
astefan merged 8 commits intoelastic:mainfrom
Conversation
AND when scoring is needed; wrap the request filter with a bool filter in case the query itself doesn't need scoring.
elasticsearchmachine
added
needs:triage
astefan
commented
Mar 4, 2025
| tsid = attr; | ||
| } else if (name.equals(MetadataAttribute.TIMESTAMP_FIELD)) { | ||
| timestamp = attr; | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Just refactoring to eliminate double iteration over plan.output() attributes.
astefan
commented
Mar 4, 2025
| Query queryDSL = TRANSLATOR_HANDLER.asQuery(Predicates.combineAnd(newPushable)); | ||
| QueryBuilder planQuery = queryDSL.asBuilder(); | ||
| var query = Queries.combine(Queries.Clause.FILTER, asList(queryExec.query(), planQuery)); | ||
| Queries.Clause combiningQueryClauseType; |
Contributor
Author
There was a problem hiding this comment.
This is the actual fix for the bug. Need to add tests.
astefan
commented
Mar 4, 2025
| assert esQueryExec.estimatedRowSize() != null : "estimated row size not initialized"; | ||
| int rowEstimatedSize = esQueryExec.estimatedRowSize(); | ||
| int limit = esQueryExec.limit() != null ? (Integer) esQueryExec.limit().fold(context.foldCtx()) : NO_LIMIT; | ||
| boolean scoring = esQueryExec.attrs() |
elasticsearchmachine
added
Team:Analytics
Collaborator
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Collaborator
|
Hi @astefan, I've created a changelog YAML for you. |
costin
reviewed
Mar 5, 2025
Member
costin
left a comment
costin
left a comment
There was a problem hiding this comment.
LGTM.
Since ES optimizes must queries into filters, a simpler fix would be to just always use the MUST clause instead and let ES handle it accordingly.
I'm thinking of the latter since it eliminates code on our side AND in time, it might evaluate if scoring is needed then we do (by searching for the score attribute which might not be sufficient in the future).
| QueryBuilder planQuery = queryDSL.asBuilder(); | ||
| var query = Queries.combine(Queries.Clause.FILTER, asList(queryExec.query(), planQuery)); | ||
| Queries.Clause combiningQueryClauseType; | ||
| if (queryExec.hasScoring()) { |
Member
There was a problem hiding this comment.
clauseType = queryExec.hasScoring() ? MUST : FILTER
| } | ||
|
|
||
| public boolean hasScoring() { | ||
| return attrs().stream().anyMatch(a -> a instanceof MetadataAttribute && a.name().equals(MetadataAttribute.SCORE)); |
Member
There was a problem hiding this comment.
Expressions.anyMatch(a -> a ..) -> not just a one liner which avoids the stream stack pollution but it also handles the expression tree navigation.
Not needed here but doesn't hurt.
astefan
added
v8.18.1
v8.19.0
v9.0.1
v8.17.4
auto-backport
Collaborator
astefan
added a commit
to astefan/elasticsearch
that referenced
this pull request
Mar 5, 2025
…coring is also needed (elastic#124001) * Use a "must" instead of "filter" when building the pushed down filter AND when scoring is needed
astefan
added a commit
to astefan/elasticsearch
that referenced
this pull request
Mar 5, 2025
…coring is also needed (elastic#124001) * Use a "must" instead of "filter" when building the pushed down filter AND when scoring is needed
astefan
added a commit
to astefan/elasticsearch
that referenced
this pull request
Mar 5, 2025
…coring is also needed (elastic#124001) * Use a "must" instead of "filter" when building the pushed down filter AND when scoring is needed
georgewallace
pushed a commit
to georgewallace/elasticsearch
that referenced
this pull request
Mar 11, 2025
…coring is also needed (elastic#124001) * Use a "must" instead of "filter" when building the pushed down filter AND when scoring is needed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #123967 by using a
muststatement (vs. afilterone) when scoring is needed in a query (enabled withmetadata _score).