ESQL: Fix some more usages of field attribute names in LOOKUP JOIN #129355

alex-spies · 2025-06-12T16:37:11Z

Add a test that demonstrates that LOOKUP JOIN works with union types.
Fix usage of FieldAttribute#name where we should use FieldAttribute#fieldName in LOOKUP JOIN.
Refactor LookupJoinTypesIT

This is not a bug fix - it's future-proofing.

LOOKUP JOIN with union types already works fine due to the fact that the union type field attributes never make it into the Join node. E.g. for FROM indices* | EVAL field = field::integer with a multi-typed field we create a field attribute called $$field$$converted_to$integer - but if we add the command LOOKUP JOIN lookup_index ON field, field will refer to the reference attribute field created in the EVAL.

However, our implementation of LOOKUP JOIN still relies on FieldAttribute#name rather than FieldAttribute#fieldName when telling the compute engine which physical index fields to use. The latter is always correct, the former is wrong for union types. To be able to propagate union type field attributes into the join, we need to use FieldAttribute#fieldName - and that's what this PR does.

This is relevant for when LOOKUP JOIN gets generalized and starts using proper expressions, like LOOKUP JOIN lookup_index ON match_field::keyword == other_field.

Also, it's beneficial not to rely on FieldAttribute#name when telling the compute engine which fields to fetch from the lookup index. It means we can handily rename the field attributes while still referring to the same physical fields.

In LOOKUP JOIN, union type fields don't work as match fields because we extract their names wrong. Correct this and make MatchConfig rely on FieldName to avoid this problem going forward.

elasticsearchmachine · 2025-06-12T16:37:43Z

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

fang-xing-esql · 2025-06-13T00:24:08Z

...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java

        private MatchConfig(FieldAttribute match, Layout.ChannelAndType input) {
            // Note, this handles TEXT fields with KEYWORD subfields
-            this(match.exactAttribute().name(), input.channel(), input.type());
+            this(match.exactAttribute().fieldName(), input.channel(), input.type());


It is not quite obvious to me how the keyword subfield of a text field works with multi-typed field, it will be great if we have a test to exercise this code path, or some comments for when match.exactAttribute().name() is different from match.exactAttribute().fieldName().

Honestly, I think this line is super redundant. The matchfield is a field from the lookup index. We can't even use TEXT as a lookup key in the lookup index, it's only allowed in the main index. So the dropping down to the exact subfield just doesn't ever apply.

I'll expand the comment. Testing this code path is currently not really possible (at least not in a realistic scenario.)

In any case, using fieldName over name is going to be more correct once the call to exactAttribute stops being redundant.

fang-xing-esql · 2025-06-13T00:56:25Z

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

+# union type behavior
+###############################################
+
+joinOnMultiTypedMatchFieldCastToInteger


Thanks for adding tests for multi-typed numeric fields as join keys. We still have gaps in test coverage for multi-typed join keys involving string types (keyword, text, ip) and date types (millis, nanos). If these are not within the scope of this PR, they should be covered in a separate one.

Added a more complete list of tests to LookupJoinTypesIT. It's still not exhaustive but IMO covers enough ground for now.

That required a bit of refactoring ^^

Use names that are easier to follow and require less explanation.

This reverts commit a3da244.

…ion-types

alex-spies · 2025-06-13T15:06:52Z

...esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java

+        // Union types; non-exhaustive and can be extended
+        {
+            TestConfigs configs = testConfigurations.computeIfAbsent("union-types", TestConfigs::new);
+            configs.addUnionTypePasses(SHORT, INTEGER, INTEGER);
+            configs.addUnionTypePasses(BYTE, DOUBLE, LONG);
+            configs.addUnionTypePasses(DATETIME, DATE_NANOS, DATE_NANOS);
+            configs.addUnionTypePasses(DATE_NANOS, DATETIME, DATETIME);
+            configs.addUnionTypePasses(SCALED_FLOAT, HALF_FLOAT, DOUBLE);
+            configs.addUnionTypePasses(TEXT, KEYWORD, KEYWORD);
+        }
+


Union-type test cases added here!

fang-xing-esql · 2025-06-13T18:26:47Z

...esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java

@@ -270,16 +241,16 @@ protected Collection<Class<? extends Plugin>> nodePlugins() {
        Set<String> knownTypes = new HashSet<>();
        for (TestConfigs configs : testConfigurations.values()) {
            for (TestConfig config : configs.configs.values()) {
-                if (knownTypes.contains(config.indexName())) {
-                    throw new IllegalArgumentException("Duplicate index name: " + config.indexName());
+                if (knownTypes.contains(config.lookupIndexName())) {


Currently, we only enforce uniqueness for lookupIndexName. If this restriction is intended to apply only to lookup indices, it might be helpful to update the error message to make that explicit—for example:
"Duplicate lookup index name:"

I'll update the message!

I think long term, we should refactor the test setup as it creates too many indices. We only really need 1 main index (with a field for every relevant type), 1 lookup index (again, 1 field for every relevant type) and only additional indices for union type cases (because we really need mapping conflicts there).

That would simplify the index creation, validation etc. I initially wanted to do this change in this PR, but I already introduced a lot of changes so that's better to do another time IMHO.

...esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java

fang-xing-esql

Thanks for adding more tests around union-typed fields, @alex-spies! I’ve added a few comments in LookupJoinTypesIT, mostly trying to improve readability.

Co-authored-by: Nhat Nguyen <[email protected]>

alex-spies · 2025-06-16T09:02:20Z

~~I included @dnhatn 's fix from #129437 since we're touching the same code and will immediately create conflicts for one another.~~

Nevermind, Nhat got his fix in before me - but I'll deal with the merge conflicts :)

…ion-types

fang-xing-esql

LGTM, thank you @alex-spies !

elasticsearchmachine · 2025-06-17T08:33:30Z

💔 Backport failed

Status	Branch	Result
❌	8.19	Commit could not be cherrypicked due to conflicts
❌	9.0	Commit could not be cherrypicked due to conflicts
❌	8.18	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 129355

…lastic#129355) * Add a test that demonstrates that LOOKUP JOIN works with union types. * Fix usage of FieldAttribute#name where we should use FieldAttribute#fieldName in LOOKUP JOIN. * Refactor LookupJoinTypesIT This doesn't fix any bugs per se, but will avoid bwc problems once we propagate union type field attributes into Join LogicalPlan nodes. (cherry picked from commit 65cbccc) # Conflicts: # x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupFromIndexIT.java # x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/FullTextFunction.java

…lastic#129355) * Add a test that demonstrates that LOOKUP JOIN works with union types. * Fix usage of FieldAttribute#name where we should use FieldAttribute#fieldName in LOOKUP JOIN. * Refactor LookupJoinTypesIT This doesn't fix any bugs per se, but will avoid bwc problems once we propagate union type field attributes into Join LogicalPlan nodes. (cherry picked from commit 65cbccc) # Conflicts: # x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java

alex-spies · 2025-06-17T16:40:52Z

💚 All backports created successfully

Status	Branch	Result
✅	9.0
✅	8.19
✅	8.18

Questions ?

Please refer to the Backport tool documentation

…lastic#129355) * Add a test that demonstrates that LOOKUP JOIN works with union types. * Fix usage of FieldAttribute#name where we should use FieldAttribute#fieldName in LOOKUP JOIN. * Refactor LookupJoinTypesIT This doesn't fix any bugs per se, but will avoid bwc problems once we propagate union type field attributes into Join LogicalPlan nodes. (cherry picked from commit 65cbccc) # Conflicts: # x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupFromIndexIT.java # x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/FullTextFunction.java

…129355) (#129577) * Add a test that demonstrates that LOOKUP JOIN works with union types. * Fix usage of FieldAttribute#name where we should use FieldAttribute#fieldName in LOOKUP JOIN. * Refactor LookupJoinTypesIT This doesn't fix any bugs per se, but will avoid bwc problems once we propagate union type field attributes into Join LogicalPlan nodes. (cherry picked from commit 65cbccc) # Conflicts: # x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupFromIndexIT.java # x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/FullTextFunction.java

…129355) (#129579) * Add a test that demonstrates that LOOKUP JOIN works with union types. * Fix usage of FieldAttribute#name where we should use FieldAttribute#fieldName in LOOKUP JOIN. * Refactor LookupJoinTypesIT This doesn't fix any bugs per se, but will avoid bwc problems once we propagate union type field attributes into Join LogicalPlan nodes. (cherry picked from commit 65cbccc) # Conflicts: # x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupFromIndexIT.java # x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/fulltext/FullTextFunction.java

…129355) (#129578) * Add a test that demonstrates that LOOKUP JOIN works with union types. * Fix usage of FieldAttribute#name where we should use FieldAttribute#fieldName in LOOKUP JOIN. * Refactor LookupJoinTypesIT This doesn't fix any bugs per se, but will avoid bwc problems once we propagate union type field attributes into Join LogicalPlan nodes. (cherry picked from commit 65cbccc) # Conflicts: # x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java

alex-spies added 4 commits June 12, 2025 18:18

Fix some attr.name() usages that meant field names

08be01c

Allow union type fields to be match fields

44dca94

In LOOKUP JOIN, union type fields don't work as match fields because we extract their names wrong. Correct this and make MatchConfig rely on FieldName to avoid this problem going forward.

Improve some comments

35ed8ea

Add csv test for LU JOIN with union types

646f378

alex-spies requested a review from fang-xing-esql June 12, 2025 16:37

alex-spies added >refactoring auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v8.19.0 v9.1.0 v9.0.3 v8.18.3 labels Jun 12, 2025

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jun 12, 2025

elasticsearchmachine and others added 2 commits June 12, 2025 16:46

[CI] Auto commit changes from spotless

e3ef5c2

Fix test compilation

8dfd4dc

fang-xing-esql reviewed Jun 13, 2025

View reviewed changes

alex-spies added 10 commits June 13, 2025 09:29

One more time

4858b1d

Simplify setup in LookupJoinTypesIT

4db8c81

Use names that are easier to follow and require less explanation.

Refactor some more

a3da244

Revert "Refactor some more"

82d359b

This reverts commit a3da244.

Expand comment

0a87d8b

Refactor some more

6634289

Refactor some more

3470281

Simplify TestConfigPasses

213f5df

Add union type test cases to LookupJoinTypesIT

77c5835

Merge remote-tracking branch 'upstream/main' into fix-lu-join-with-un…

7f98271

…ion-types

alex-spies commented Jun 13, 2025

View reviewed changes

fang-xing-esql reviewed Jun 13, 2025

View reviewed changes

...esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java Outdated Show resolved Hide resolved

fang-xing-esql reviewed Jun 13, 2025

View reviewed changes

...esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/LookupJoinTypesIT.java Outdated Show resolved Hide resolved

fang-xing-esql reviewed Jun 13, 2025

View reviewed changes

Improve readability based on Fang's comments

db3b5bb

alex-spies mentioned this pull request Jun 16, 2025

Avoid HandleLimitFS in LookupJoinTypesIT #129437

Merged

Avoid HandleLimitFS in LookupJoinTypesIT

e86539b

Co-authored-by: Nhat Nguyen <[email protected]>

alex-spies requested a review from fang-xing-esql June 16, 2025 09:02

[CI] Auto commit changes from spotless

9d4102e

alex-spies requested review from dnhatn June 16, 2025 09:08

alex-spies mentioned this pull request Jun 16, 2025

ESQL: Enable pushing down LOOKUP JOIN past Project #127776

Closed

alex-spies added 3 commits June 16, 2025 17:25

Merge remote-tracking branch 'upstream/main' into fix-lu-join-with-un…

bf01b0d

…ion-types

Make naming more consistent

f0f4b48

Improve error message

3a8594e

fang-xing-esql approved these changes Jun 16, 2025

View reviewed changes

alex-spies merged commit 65cbccc into elastic:main Jun 17, 2025
24 checks passed

alex-spies deleted the fix-lu-join-with-union-types branch June 17, 2025 08:30

elasticsearchmachine added the backport pending label Jun 17, 2025

alex-spies mentioned this pull request Jun 17, 2025

[9.0] ESQL: Fix some more usages of field attribute names in LOOKUP JOIN (#129355) #129577

Merged

alex-spies mentioned this pull request Jun 17, 2025

[8.19] ESQL: Fix some more usages of field attribute names in LOOKUP JOIN (#129355) #129578

Merged

alex-spies mentioned this pull request Jun 17, 2025

[8.18] ESQL: Fix some more usages of field attribute names in LOOKUP JOIN (#129355) #129579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ESQL: Fix some more usages of field attribute names in LOOKUP JOIN #129355

ESQL: Fix some more usages of field attribute names in LOOKUP JOIN #129355

Uh oh!

alex-spies commented Jun 12, 2025 •

edited

Loading

Uh oh!

elasticsearchmachine commented Jun 12, 2025

Uh oh!

fang-xing-esql Jun 13, 2025

Uh oh!

alex-spies Jun 13, 2025

Uh oh!

fang-xing-esql Jun 13, 2025

Uh oh!

alex-spies Jun 13, 2025

Uh oh!

alex-spies Jun 13, 2025

Uh oh!

fang-xing-esql Jun 13, 2025 •

edited

Loading

Uh oh!

alex-spies Jun 16, 2025

Uh oh!

Uh oh!

Uh oh!

fang-xing-esql left a comment

Uh oh!

alex-spies commented Jun 16, 2025 •

edited

Loading

Uh oh!

fang-xing-esql left a comment

Uh oh!

Uh oh!

elasticsearchmachine commented Jun 17, 2025

Uh oh!

alex-spies commented Jun 17, 2025

Uh oh!

Uh oh!

ESQL: Fix some more usages of field attribute names in LOOKUP JOIN #129355

ESQL: Fix some more usages of field attribute names in LOOKUP JOIN #129355

Uh oh!

Conversation

alex-spies commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jun 12, 2025

Uh oh!

fang-xing-esql Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

alex-spies Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

fang-xing-esql Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

alex-spies Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

alex-spies Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

fang-xing-esql Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-spies Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fang-xing-esql left a comment

Choose a reason for hiding this comment

Uh oh!

alex-spies commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fang-xing-esql left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Jun 17, 2025

💔 Backport failed

Uh oh!

alex-spies commented Jun 17, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Uh oh!

alex-spies commented Jun 12, 2025 •

edited

Loading

fang-xing-esql Jun 13, 2025 •

edited

Loading

alex-spies commented Jun 16, 2025 •

edited

Loading