Enhance LOOKUP JOIN csv-spec tests to cover more cases and fix several bugs found #117843

craigtaverner · 2024-12-02T18:05:48Z

Adds several more tests to lookup-join.csv-spec, and fixes the following bugs:

FieldCaps on right hand side should ignore fieldNames method and just use "*" because currently the fieldNames search cannot handle lookup fields with aliases (should be fixed in a followup PR).
Stop using the lookup index in the ComputeService (so we don’t get both indices data coming in from the left, and other weird behaviour).
Ignore failing SearchStats checks on fields from the right hand side in the logical planner (so it does not plan EVAL field = null for all right hand fields). This should be fixed properly with the correct updates to TransportSearchShardsAction (or rather to making multiple use of that for each branch of the execution model).

Adds several more tests to lookup-join.csv-spec, and fixes the following bugs: * FieldCaps on right hand side should ignore fieldNames method and just use "*" because we don’t specify the right hand side fields at all in LOOKUP JOIN (presumably we will in future, and then we can change this). * Stop using the lookup index in the ComputeService (so we don’t get both index data coming in from the left, and other weird behaviour). * Ignore failing SearchStats checks on fields from the right hand side in the logical planner (so it does not plan EVAL field = null for all right hand fields). This should be fixed properly with the correct updates to TransportSearchShardsAction (or rather to making multiple use of that for each branch of the execution model).

elasticsearchmachine · 2024-12-02T18:06:13Z

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

And re-enable one disabled test

costin

LGTM. There are a number of culprits but the PR nicely avoids them while adding minimal changes.

costin · 2024-12-03T04:57:28Z

.../java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/InsertFieldExtraction.java

@@ -102,15 +101,18 @@ public PhysicalPlan apply(PhysicalPlan plan) {

    private static Set<Attribute> missingAttributes(PhysicalPlan p) {
        var missing = new LinkedHashSet<Attribute>();
-        var inputSet = p.inputSet();
+        var input = new AttributeSet(p.inputSet());


Why make another copy?

costin · 2024-12-03T04:57:46Z

.../java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/InsertFieldExtraction.java


-        // TODO: We need to extract whatever fields are missing from the left hand side.
-        // skip the lookup join since the right side is always materialized and a projection
+        // For LOOKUP JOIN we only need field-extraction on left fields used to match, since the right side is always materialized


FTR: we want to get field extraction on the right side as well at some point.

The field extraction is implicitly done inside the LookupFromIndexService. I agree it'd be nicer to make the field extraction visible in the physical plan; currently, that'd have no benefit, though, because we cannot really materialize fields late - all looked up fields need to be extracted inside the LookupFromIndexService.

It'd require a fundamental change of the execution to extract additional fields after the execution of the lookup join.

costin · 2024-12-03T05:01:18Z

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

+    private Set<String> findLookupIndexNames(PhysicalPlan physicalPlan) {
+        Set<String> lookupIndexNames = new HashSet<>();
+        physicalPlan.forEachDown(
+            LookupJoinExec.class,
+            lookupJoinExec -> lookupJoinExec.lookup().forEachDown(EsQueryExec.class, es -> lookupIndexNames.add(es.index().name()))
+        );
+        physicalPlan.forEachDown(
+            LookupJoinExec.class,
+            lookupJoinExec -> lookupJoinExec.lookup()
+                .forEachDown(
+                    FragmentExec.class,
+                    frag -> frag.fragment().forEachDown(EsRelation.class, esRelation -> lookupIndexNames.add(esRelation.index().name()))
+                )
+        );
+        // TODO this only works for LEFT join, so we still need to support RIGHT join
+        physicalPlan.forEachDown(
+            FragmentExec.class,
+            fragmentExec -> fragmentExec.fragment()
+                .forEachDown(
+                    Join.class,
+                    join -> join.right().forEachDown(EsRelation.class, esRelation -> lookupIndexNames.add(esRelation.index().name()))
+                )
+        );
+        return lookupIndexNames;


Combine the iterations into one.

elasticsearchmachine · 2024-12-03T14:12:08Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

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

craigtaverner · 2024-12-03T14:21:30Z

...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/LookupFromIndexService.java

        return termQueryList(fieldType, context, inputBlock, inputDataType);
    }

+    private static void validateTypes(DataType inputDataType, MappedFieldType fieldType) {


Not so much a bug-fix as early error generation. This could be made less strict going forward, but we thought to start strict and then open later if appropriate.

craigtaverner · 2024-12-03T14:22:43Z

.../org/elasticsearch/xpack/esql/optimizer/rules/logical/local/ReplaceMissingFieldWithNull.java


            for (NamedExpression projection : projections) {
                // Do not use the attribute name, this can deviate from the field name for union types.
-                if (projection instanceof FieldAttribute f && stats.exists(f.fieldName()) == false) {
+                if (projection instanceof FieldAttribute f && stats.exists(f.fieldName()) == false && joinAttributes.contains(f) == false) {


This is a temporary fix to the fact that SearchStats can currently only contain all stats for the main FROM index part of the query. We need a full-stack change of context to support multiple SearchStats.

craigtaverner · 2024-12-03T14:23:51Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/PlannerUtils.java

@@ -117,12 +119,17 @@ public static String[] planOriginalIndices(PhysicalPlan plan) {
        var indices = new LinkedHashSet<String>();
        plan.forEachUp(
            FragmentExec.class,
-            f -> f.fragment()
-                .forEachUp(EsRelation.class, r -> indices.addAll(asList(Strings.commaDelimitedListToStringArray(r.index().name()))))
+            f -> f.fragment().forEachUp(EsRelation.class, r -> addOriginalIndexIfNotLookup(indices, r.index()))


Part of the fix to remove join indexes from the left-hand-side of the join (ie. stop mixing left and right)

craigtaverner · 2024-12-03T14:24:13Z

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

@@ -160,9 +165,11 @@ public void execute(
        Map<String, OriginalIndices> clusterToConcreteIndices = transportService.getRemoteClusterService()
            .groupIndices(SearchRequest.DEFAULT_INDICES_OPTIONS, PlannerUtils.planConcreteIndices(physicalPlan).toArray(String[]::new));
        QueryPragmas queryPragmas = configuration.pragmas();
+        Set<String> lookupIndexNames = findLookupIndexNames(physicalPlan);
+        Set<String> concreteIndexNames = selectConcreteIndices(clusterToConcreteIndices, lookupIndexNames);


Part of the fix to remove join indexes from the left-hand-side of the join (ie. stop mixing left and right)

craigtaverner · 2024-12-03T14:25:01Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java

@@ -313,7 +313,7 @@ private <T> void preAnalyze(
            // First resolve the lookup indices, then the main indices
            preAnalyzeLookupIndices(
                preAnalysis.lookupIndices,
-                fieldNames,
+                Set.of("*"), // Current LOOKUP JOIN syntax does not allow for field selection


Temporary fix to work around a bug in fieldNames for lookup-join when there are aliases before, and keep after the lookup.

alex-spies

Thanks @craigtaverner , this is a very, very nice PR.

I added some late comments to have a reference for follow-ups.

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

alex-spies · 2024-12-03T14:44:35Z

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

+
+FROM sample_data
+| LOOKUP JOIN message_types_lookup ON message
+| KEEP @timestamp, client_ip, event_duration, message, type


Maybe we could also try a different order/subset of columns for good measure

yes, good idea. I did have issues with column ordering in the layout, so...

I'll open a PR with more tests.

alex-spies · 2024-12-03T14:45:03Z

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

+ROW left = "left", client_ip = "172.21.0.5", env = "env", right = "right"
+| EVAL client_ip = client_ip::keyword
+| LOOKUP JOIN clientips_lookup ON client_ip
+| KEEP left, client_ip, right, env


Similarly here.

alex-spies · 2024-12-03T14:52:42Z

...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/LookupFromIndexService.java

+        validateTypes(request.inputDataType, fieldType);
        return termQueryList(fieldType, context, inputBlock, inputDataType);
    }

+    private static void validateTypes(DataType inputDataType, MappedFieldType fieldType) {
+        // TODO: consider supporting implicit type conversion as done in ENRICH for some types
+        if (fieldType.typeName().equals(inputDataType.typeName()) == false) {
+            throw new EsqlIllegalArgumentException(
+                "LOOKUP JOIN match and input types are incompatible: match[" + fieldType.typeName() + "], input[" + inputDataType + "]"
+            );
+        }
+    }


We should perform this kind of validation during query planning and should return 400 not 500.

Yes, the same is true for ENRICH. And for ENRICH we have both validation during planning and at this point, and the planning one is less comprehensive because less is known at that point. We should check what is known for JOIN; and if we can move this entirely to planning.

alex-spies · 2024-12-03T14:56:40Z

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

@@ -565,6 +565,7 @@ private PhysicalOperation planHashJoin(HashJoinExec join, LocalExecutionPlannerC

    private PhysicalOperation planLookupJoin(LookupJoinExec join, LocalExecutionPlannerContext context) {
        PhysicalOperation source = plan(join.left(), context);
+        // TODO: The source builder includes incoming fields including the ones we're going to drop


That's indeed weird but I don't think it's an incorrect layout. The physical operation represented by the LookupJoinExec actually can only append blocks - this means that physically, pages will still contain shadowed blocks until we pass through a project operator that actually strips them.

It's the same for Eval and Enrich. The physical operators usually do not drop columns.

The fact that layouts are based on name ids instead of names makes it so this isn't incorrect.

This comment was added very early on during discussions with you. We added it as a reminder. Could it be that it went stale immediately, since you did your fixes? This should be verified.

It's always been stale, I just didn't understand the physical operators enough at the time :D

If you look here, you can see that the corresponding operator will only ever append blocks from the right.

I thought the layout was wrong because it would not take into account shadowing. But that's actually fine! Shadowing is a logical concept, the physical operators don't care. They only care about channels and which data is present in which. And for this, the layout constructed here appears to be very correct: it has the channels from the left hand side and additionally more channels slapped onto it from the right, containing the looked up fields.

.../org/elasticsearch/xpack/esql/optimizer/rules/logical/local/ReplaceMissingFieldWithNull.java

alex-spies · 2024-12-03T17:02:05Z

.../java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/InsertFieldExtraction.java


-        // TODO: We need to extract whatever fields are missing from the left hand side.
-        // skip the lookup join since the right side is always materialized and a projection
+        // For LOOKUP JOIN we only need field-extraction on left fields used to match, since the right side is always materialized


The field extraction is implicitly done inside the LookupFromIndexService. I agree it'd be nicer to make the field extraction visible in the physical plan; currently, that'd have no benefit, though, because we cannot really materialize fields late - all looked up fields need to be extracted inside the LookupFromIndexService.

It'd require a fundamental change of the execution to extract additional fields after the execution of the lookup join.

alex-spies · 2024-12-03T17:07:09Z

.../java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/InsertFieldExtraction.java


-        // TODO: We need to extract whatever fields are missing from the left hand side.
-        // skip the lookup join since the right side is always materialized and a projection
+        // For LOOKUP JOIN we only need field-extraction on left fields used to match, since the right side is always materialized
        if (p instanceof LookupJoinExec join) {


As discussed, I think this can be simplified by just using p.references instead of having a special case for lookup joins, and otherwise doing something on p.forEacExpression(...).

alex-spies · 2024-12-03T17:22:40Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java

@@ -313,7 +313,7 @@ private <T> void preAnalyze(
            // First resolve the lookup indices, then the main indices
            preAnalyzeLookupIndices(
                preAnalysis.lookupIndices,
-                fieldNames,
+                Set.of("*"), // Current LOOKUP JOIN syntax does not allow for field selection


I think the comment is insufficient; I believe this is a good hack but should be improved later; in a query like

FROM idx | LOOKUP JOIN lookup_idx ON lookup_field | KEEP another_lookup_field

we do not have to ask for all existing field names. We just need another_lookup_field.

In fact, I think this will make it so that we ask the LookupFromIndexService to fetch all fields from the index, meaning that the performance of this operation will depend on the number of fields in the lookup index. In scenarios where that index has, say, 6000 fields (not completely unheard of), that may be a serious performance drag and could cause memory issues.

To be more precise, I think we'll have to augmentfieldNames. We should be able to walk the tree from the top, stopping at the first LOOKUP JOIN we see - and then determining that the missing fields so far are the only candidates that may come from the lookup index. This means that the output of fieldNames shouldn't be a single Set<String> - but, instead, a Set<String> for the main index and a List<Set<String>> corresponding to the missing fields we might obtain from the lookup indices.

The comment in the followup work is:

EsqlSession.fieldNames does not handle lookup references that are also mentioned in aliases (erases them)

So we want to fix this bug, and I have a reasonable idea how.

alex-spies · 2024-12-03T17:27:24Z

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

+
+    private Set<String> findLookupIndexNames(PhysicalPlan physicalPlan) {
+        Set<String> lookupIndexNames = new HashSet<>();
+        // When planning JOIN on the coordinator node: "LookupJoinExec.lookup()->FragmentExec.fragment()->EsRelation.index()"


Super useful comments!

I actually ran a test that checked which of the 19 queries hit which of these two patterns, just to be sure, and the trend was pretty clear.

craigtaverner 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 Dec 2, 2024

craigtaverner requested review from costin and alex-spies December 2, 2024 18:05

Use new unique join_lookup_v4 EsqlCapability

ca9c827

And re-enable one disabled test

costin approved these changes Dec 3, 2024

View reviewed changes

craigtaverner added 5 commits December 3, 2024 10:33

Merge branch 'main' into fix-lookupjoin

d4b046b

Merge branch 'main' into fix-lookupjoin

7082bb8

Refine search for lookup indices based on code review

c600e6f

Merge remote-tracking branch 'origin/main' into fix-lookupjoin

97311f3

Remove unnecessary copy of AttributeSet

1fd56e7

craigtaverner changed the title ~~Fix several LOOKUP JOIN bugs~~ Enhance LOOKUP JOIN csv-spec tests to cover mode cases and fix several bugs found Dec 3, 2024

craigtaverner mentioned this pull request Dec 3, 2024

ESQL: Lookup Join meta issue #116208

Open

36 tasks

craigtaverner changed the title ~~Enhance LOOKUP JOIN csv-spec tests to cover mode cases and fix several bugs found~~ Enhance LOOKUP JOIN csv-spec tests to cover more cases and fix several bugs found Dec 3, 2024

craigtaverner mentioned this pull request Dec 3, 2024

ESQL: LOOKUP JOIN produces additional null rows #117702

Closed

craigtaverner merged commit d3f0ae0 into elastic:main Dec 3, 2024
16 checks passed

elasticsearchmachine added the backport pending label Dec 3, 2024

craigtaverner commented Dec 3, 2024

View reviewed changes

alex-spies reviewed Dec 3, 2024

View reviewed changes

alex-spies mentioned this pull request Dec 3, 2024

ESQL: Small LOOKUP JOIN cleanups #117922

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance LOOKUP JOIN csv-spec tests to cover more cases and fix several bugs found #117843

Enhance LOOKUP JOIN csv-spec tests to cover more cases and fix several bugs found #117843

craigtaverner commented Dec 2, 2024 •

edited

Loading

elasticsearchmachine commented Dec 2, 2024

costin left a comment

costin Dec 3, 2024

costin Dec 3, 2024

alex-spies Dec 3, 2024

costin Dec 3, 2024

elasticsearchmachine commented Dec 3, 2024

craigtaverner Dec 3, 2024

craigtaverner Dec 3, 2024

craigtaverner Dec 3, 2024

craigtaverner Dec 3, 2024

craigtaverner Dec 3, 2024

alex-spies left a comment

alex-spies Dec 3, 2024

craigtaverner Dec 3, 2024

alex-spies Dec 3, 2024

alex-spies Dec 3, 2024

alex-spies Dec 3, 2024

craigtaverner Dec 3, 2024

alex-spies Dec 3, 2024

craigtaverner Dec 3, 2024

alex-spies Dec 3, 2024

alex-spies Dec 3, 2024

alex-spies Dec 3, 2024

alex-spies Dec 3, 2024

craigtaverner Dec 3, 2024

alex-spies Dec 3, 2024

craigtaverner Dec 3, 2024

Enhance LOOKUP JOIN csv-spec tests to cover more cases and fix several bugs found #117843

Enhance LOOKUP JOIN csv-spec tests to cover more cases and fix several bugs found #117843

Conversation

craigtaverner commented Dec 2, 2024 • edited Loading

elasticsearchmachine commented Dec 2, 2024

costin 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

elasticsearchmachine commented Dec 3, 2024

💔 Backport failed

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

alex-spies 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

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

Choose a reason for hiding this comment

craigtaverner commented Dec 2, 2024 •

edited

Loading