ESQL: Ignore multivalued key columns in lookup index on JOIN #120726

ivancea · 2025-01-23T15:04:06Z

Second part of #120519

In the first PR, we avoid matching multivalue keys in lookup when they come from the query.
Now, we avoid matching multivalues when the lookup index has multivalues in the key column.

elasticsearchmachine · 2025-01-23T15:04:31Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

ivancea · 2025-01-23T15:06:05Z

...k/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/lookup/QueryList.java

+            }
+        } catch (IOException e) {
+            // ignore
+            // TODO: Should we do something with the exception?


Maybe wrap it into a runtime exception?

Rethrowing it as unchecked

Wrap it, yeah. ESQL has kind of a rocky relationship with IOException. Pretty much everything should be allowed to throw it but we started wrapping it long ago and never stopped.

FWIW this can throw if there's an IO error blowing up on the disk with lucene. It's not likely and probably terminal for the query. But we should bubble it up.

ivancea · 2025-01-23T15:07:01Z

...k/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/lookup/QueryList.java

-            }
-            final int first = block.getFirstValueIndex(position);
-            return switch (count) {
+        Query doGetQuery(int position, int firstValueIndex, int valueCount) {


Those params are oddly specific, but at least we won't have to get them twice...

ivancea · 2025-01-23T15:08:03Z

...ql/compute/src/main/java/org/elasticsearch/compute/querydsl/query/SingleValueMatchQuery.java

This was in the main esql project, and we don't have access from compute. So as to avoid cloning the query class, and as it doesn't use anything other than compute and lucene classes, I moved it

ivancea · 2025-01-23T15:08:43Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec

Didn't add new tests, as the existing ones failed successfully. So just updated them

ivancea · 2025-01-23T15:09:20Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

+        /**
+         * LOOKUP JOIN without MV matching on lookup index key (https://github.com/elastic/elasticsearch/issues/118780)
+         */
+        JOIN_LOOKUP_SKIP_MV_ON_LOOKUP_KEY(JOIN_LOOKUP_V11.isEnabled()),


To avoid renaming conficts, I'm adding a new cap. We can remove it later if we want

astefan · 2025-01-23T15:38:01Z

...k/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/lookup/QueryList.java

+        SingleValueMatchQuery singleValueQuery = new SingleValueMatchQuery(
+            searchExecutionContext.getForField(field, MappedFieldType.FielddataOperation.SEARCH),
+            // Not emitting warnings for multivalued fields not matching
+            Warnings.NOOP_WARNINGS


I don't think we should just ignore this behavior. Today we do emit a warning if there's an equality operation involving a multi-value field, ie row x=[1,2,3],y=3 | where x == y. The joining key matching is basically doing the same thing, no?

The logic is the same, indeed.
I have doubts over the warnings here, as I thought that it would be "clear enough" in JOIN.
That said, yeah, we could use that argument for everything else, so I'll better add warnings. I'll customize them, as SingleValueMatchQuery sends a generic single-value function encountered multi-value. I'll add something more specific for JOIN

After some investigation and things, I have some local changes with this:

Warnings are being correctly generated, nice!

They are sent to coordinator correctly

BUT the AsyncOperator (parent of the Lookup operator) doesn't merge them with the main ThreadLocal

It's not trivial to solve (Or at least, is a quite problematic topic). After commenting it a bit with Nik, we'll better leave the Warnings part for another PR.
If there's time to make it for the FF, that's fine. Otherwise, should be fine too (?). I'll keep working on it anyway.

Leaving a comment around warnings that should be addressed as a follow-up: we should limit the amount of warnings an operator throws for cases to avoid flooding the system with objects that end up being dropped due to the limit being reached - either by a single operator, or by multiple operators, either before or during the warnings being emitted.
This is similar to the logger usage where we're invoking methods without doing any args transformation in order to perform the check the logging statement first.

We do limit these warnings in other operators and think we mostly just stick to prior art here. I think it's fine to juggle them in a follow up. Honestly, I think we should rip out the thread local handling of warnings from ESQL..... It's too sneaky. But I don't know if we'll have time for that for a while.

ivancea · 2025-01-24T12:43:25Z

.../src/test/java/org/elasticsearch/compute/operator/lookup/EnrichQuerySourceOperatorTests.java

@@ -277,14 +248,45 @@ private static DirectoryData makeDirectoryWith(List<List<String>> terms) throws
            for (var termList : terms) {
                Document doc = new Document();
                for (String term : termList) {
-                    doc.add(new StringField("uid", term, Field.Store.NO));
+                    doc.add(new KeywordField("uid", term, Field.Store.NO));


I had to change this as to avoid an unexpected docvalues type NONE for field error in the tests, within SingleValueMatchQuery. And add some other objects to the SearchExecutionContext mock

Right. StringField won't make doc values and we can't query them.

FWIW, I think we should make sure we fail correctly if you turn off doc values or searchability on a join key.

Hey! That also means that text fields can't be joined. They just super don't have doc values.

Just checked it, but #119473 already disallowed joins on TEXT, so it's safe for now

astefan

I am ok with this change 👍

Also, I would keep this followup (null as a warning for MVs join key matching) somewhere on a list or new issue, just that we don't forget about it and we prioritize accordingly afterwards. Thanks.

# Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec

nik9000 · 2025-01-27T15:04:55Z

...k/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/lookup/QueryList.java

+        SingleValueMatchQuery singleValueQuery = new SingleValueMatchQuery(
+            searchExecutionContext.getForField(field, MappedFieldType.FielddataOperation.SEARCH),
+            // Not emitting warnings for multivalued fields not matching
+            Warnings.NOOP_WARNINGS


We do limit these warnings in other operators and think we mostly just stick to prior art here. I think it's fine to juggle them in a follow up. Honestly, I think we should rip out the thread local handling of warnings from ESQL..... It's too sneaky. But I don't know if we'll have time for that for a while.

nik9000 · 2025-01-27T15:06:04Z

...k/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/lookup/QueryList.java

+            }
+        } catch (IOException e) {
+            // ignore
+            // TODO: Should we do something with the exception?


Wrap it, yeah. ESQL has kind of a rocky relationship with IOException. Pretty much everything should be allowed to throw it but we started wrapping it long ago and never stopped.

nik9000 · 2025-01-27T15:08:13Z

...k/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/lookup/QueryList.java

+            }
+        } catch (IOException e) {
+            // ignore
+            // TODO: Should we do something with the exception?


FWIW this can throw if there's an IO error blowing up on the disk with lucene. It's not likely and probably terminal for the query. But we should bubble it up.

nik9000 · 2025-01-27T15:09:57Z

.../src/test/java/org/elasticsearch/compute/operator/lookup/EnrichQuerySourceOperatorTests.java

@@ -277,14 +248,45 @@ private static DirectoryData makeDirectoryWith(List<List<String>> terms) throws
            for (var termList : terms) {
                Document doc = new Document();
                for (String term : termList) {
-                    doc.add(new StringField("uid", term, Field.Store.NO));
+                    doc.add(new KeywordField("uid", term, Field.Store.NO));


Right. StringField won't make doc values and we can't query them.

FWIW, I think we should make sure we fail correctly if you turn off doc values or searchability on a join key.

nik9000 · 2025-01-27T15:10:18Z

.../src/test/java/org/elasticsearch/compute/operator/lookup/EnrichQuerySourceOperatorTests.java

@@ -277,14 +248,45 @@ private static DirectoryData makeDirectoryWith(List<List<String>> terms) throws
            for (var termList : terms) {
                Document doc = new Document();
                for (String term : termList) {
-                    doc.add(new StringField("uid", term, Field.Store.NO));
+                    doc.add(new KeywordField("uid", term, Field.Store.NO));


Hey! That also means that text fields can't be joined. They just super don't have doc values.

elasticsearchmachine · 2025-01-28T13:50:42Z

💚 Backport successful

Status	Branch	Result
✅	8.x

#121037) Fixes #118780 Second part of #120519 In the first PR, we avoid matching multivalue keys in lookup when they come from the query. Now, we avoid matching multivalues when the lookup index has multivalues in the key column.

ESQL: Ignore multivalued key columns in lookup index

630b673

ivancea added >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v9.0.0 v8.18.0 labels Jan 23, 2025

ivancea requested review from nik9000 and alex-spies January 23, 2025 15:04

ivancea commented Jan 23, 2025

View reviewed changes

astefan reviewed Jan 23, 2025

View reviewed changes

Fixed test to work with Query rewrites

4692f62

ivancea commented Jan 24, 2025

View reviewed changes

ivancea requested a review from astefan January 24, 2025 17:52

astefan approved these changes Jan 27, 2025

View reviewed changes

Merge branch 'main' into esql-join-multivalue-on-index

91f2684

# Conflicts: # x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec

nik9000 approved these changes Jan 27, 2025

View reviewed changes

alex-spies mentioned this pull request Jan 27, 2025

ESQL: LOOKUP JOIN should have SQL LEFT JOIN semantics #118781

Closed

Merge branch 'main' into esql-join-multivalue-on-index

b82b446

ivancea enabled auto-merge (squash) January 28, 2025 12:41

ivancea merged commit 8c3d9e1 into elastic:main Jan 28, 2025
16 checks passed

ivancea deleted the esql-join-multivalue-on-index branch January 28, 2025 13:49

ivancea mentioned this pull request Jan 28, 2025

[8.x] ESQL: Ignore multivalued key columns in lookup index on JOIN (#120726) #121037

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Ignore multivalued key columns in lookup index on JOIN #120726

ESQL: Ignore multivalued key columns in lookup index on JOIN #120726

ivancea commented Jan 23, 2025

elasticsearchmachine commented Jan 23, 2025

ivancea Jan 23, 2025

ivancea Jan 24, 2025

nik9000 Jan 27, 2025

nik9000 Jan 27, 2025

ivancea Jan 23, 2025

ivancea Jan 23, 2025

ivancea Jan 23, 2025

ivancea Jan 23, 2025

astefan Jan 23, 2025

ivancea Jan 23, 2025 •

edited

Loading

ivancea Jan 24, 2025

costin Jan 24, 2025

nik9000 Jan 27, 2025

ivancea Jan 24, 2025

nik9000 Jan 27, 2025

nik9000 Jan 27, 2025

ivancea Jan 28, 2025

astefan left a comment

nik9000 Jan 27, 2025

nik9000 Jan 27, 2025

nik9000 Jan 27, 2025

nik9000 Jan 27, 2025

nik9000 Jan 27, 2025

elasticsearchmachine commented Jan 28, 2025

ESQL: Ignore multivalued key columns in lookup index on JOIN #120726

ESQL: Ignore multivalued key columns in lookup index on JOIN #120726

Conversation

ivancea commented Jan 23, 2025

elasticsearchmachine commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivancea Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Jan 28, 2025

💚 Backport successful

ivancea Jan 23, 2025 •

edited

Loading