ESQL: Use a must boolean statement when pushing down to Lucene when scoring is also needed #124001
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 
| tsid = attr; | ||
| } else if (name.equals(MetadataAttribute.TIMESTAMP_FIELD)) { | ||
| timestamp = attr; | ||
| |
There was a problem hiding this comment.
Just refactoring to eliminate double iteration over plan.output() attributes.
| 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; |
There was a problem hiding this comment.
This is the actual fix for the bug. Need to add tests.
| 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 
| Pinging @elastic/es-analytical-engine (Team:Analytics) |
| Hi @astefan, I've created a changelog YAML for you. |
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()) { |
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)); |
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 
…coring is also needed (elastic#124001) * Use a "must" instead of "filter" when building the pushed down filter AND when scoring is needed
…coring is also needed (elastic#124001) * Use a "must" instead of "filter" when building the pushed down filter AND when scoring is needed
…coring is also needed (elastic#124001) * Use a "must" instead of "filter" when building the pushed down filter AND when scoring is needed
…coring is also needed (elastic#124001) * Use a "must" instead of "filter" when building the pushed down filter AND when scoring is needed
4 participants
Fixes #123967 by using a
muststatement (vs. afilterone) when scoring is needed in a query (enabled withmetadata _score).