ESQL: Skip multivalues in LOOKUP JOIN matches #120519
Conversation
ivancea
added
>non-issue
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| @@ -146,7 +146,7 @@ private static void forEachFromRelation(PhysicalPlan plan, Consumer<EsRelation> | |||
| } | |||
|
|
|||
| /** | |||
| * Similar to {@link Node#forEachUp(Consumer)}, but with a custom callback to get the node children. | |||
| * Similar to {@link Node#forEachUp(Class, Consumer)}, but with a custom callback to get the node children. | |||
There was a problem hiding this comment.
This is unrelated, just a minor docs fix
| Query query = queryList.getQuery(queryPosition); | ||
| if (query != null) { | ||
| return query; | ||
| if (skipMultiValuesMatching == false || queryList.block.getValueCount(queryPosition) == 1) { |
There was a problem hiding this comment.
This queryList.block.getValueCount(queryPosition) is a bit weird for 2 reasons:
- It's a protected field, accessed here for convenience
- It's an abstract method (I'll run benchmarks to ensure this doesn't affect performance)
There was a problem hiding this comment.
I wonder if we should do this by changing TermQueryList - either by making a new one or pushing a boolean into it.
| /** | ||
| * LOOKUP JOIN without MV matching (https://github.com/elastic/elasticsearch/issues/118780) | ||
| */ | ||
| JOIN_LOOKUP_SKIP_MV(JOIN_LOOKUP_V11.isEnabled()), |
There was a problem hiding this comment.
The post-merge conflicts with the renaming approach make it very prone to get main broken. So I'll either rename it later before merging, or just remove the capability and update the other one in another PR
There was a problem hiding this comment.
That hit me a while back too. We keep bumping the same number and then everything breaks.
| Query query = queryList.getQuery(queryPosition); | ||
| if (query != null) { | ||
| return query; | ||
| if (skipMultiValuesMatching == false || queryList.block.getValueCount(queryPosition) == 1) { |
There was a problem hiding this comment.
I wonder if we should do this by changing TermQueryList - either by making a new one or pushing a boolean into it.
There was a problem hiding this comment.
Tip: Hide whitespace when reviewing this file
💔 Backport failed
You can use sqren/backport to manually backport by running |
Fixes #118780
To follow the same logics of the
==operator (Doesn't match or work at all on multivalues or nulls), we're removing the Lucene matching currently being indirectly used by LOOKUP JOIN.