Support multi-index LOOKUP JOIN and various bug fixes

[craigtaverner]

While working on supporting multi-index LOOKUP JOIN; various bugs were fixed:

• Previously we just used '*' for lookup-join indices, because the fieldnames were sometimes not being correctly determined. The problem was with KEEP referencing fields from the right that had previously been defined on the left as aliases, including using the ROW command. We normally don't want to ask for aliases, but if they could be shadowed by a lookup join, we need to keep them.
• With both single and multi-index LOOKUP JOIN we need to mark each index as potentially wildcard fields, if the KEEP commands occur before the LOOKUP JOIN.

[elasticsearchmachine]

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

[alex-spies]

Heya, I think this should improve things, but I think we need to add tests to really see what fieldnames we'll request. There are tests for the field names method and I think we should throw a bunch of LOOKUP JOIN queries in there to ensure that we're asking for the right fields. In particular, there may be bugs in situations where there's a KEEP/DROP before (i.e. upstream from) the LOOKUP JOIN, which would indicate that we only need some fields from the main index - that could end up requesting too few fields from the LOOKUP JOIN.

I tried the following test, and it passes as-is on main:

    public void testLookupJoin() {
        assertFieldNames(
            "FROM employees | KEEP languages | RENAME languages AS language_code | LOOKUP JOIN languages_lookup ON language_code",
            Set.of("languages", "language_code", "language_code.*", "languages.*")
        );
    }

Not sure that's correct - we actually need all the fields from LOOKUP JOIN.

Note: Since we supply a single list of field names that doesn't differentiate between the main index and lookup indices (c.f. here where the fieldnames are passed to preAnalyzeLookupIndices), we'll ask for all the main index' field names, too, and will request too many fields either from the main index, or from lookup indices in case of multiple lookups that shadow.

Requesting too many fields is likely okay for now as long as it's not *. We probably want an optimization rule that prunes the lookup to only fetch the relevant fields, and it shouldn't/can't be the job of the pre-analysis stage to determine this.

[astefan]

This may not be related to this specific PR's changes, it may be something that was postponned but, nevertheless, I think we need tests in IndexResolverFieldNamesTests for queries that use lookup join queries so that the fieldNames output is correct given those queries. This may be the right PR to add the missing tests.

The meta issue also mentions a bug related to fieldNames. I am not sure if what this PR is doing has any relation to that bug, but the lack of any lookup join related tests in IndexResolverFieldNamesTests is concerning.

[craigtaverner]

This may not be related to this specific PR's changes, it may be something that was postponned but, nevertheless, I think we need tests in IndexResolverFieldNamesTests for queries that use lookup join queries so that the fieldNames output is correct given those queries. This may be the right PR to add the missing tests.

I could investigate that. The current PR replaces a fix that was already tested in the csv-spec tests, with a new fix. I did verify that removing "*" without also fixing the alias check causes failures.

The meta issue also mentions a bug related to fieldNames. I am not sure if what this PR is doing has any relation to that bug, but the lack of any lookup join related tests in IndexResolverFieldNamesTests is concerning.

The bug in the meta-issue was added by me, so yes, this is the fix for that bug. The text of the bug is "EsqlSession.fieldNames does not handle lookup references that are also mentioned in aliases (erases them)", but no specific issue was created, since it is a refactoring mainly.

[astefan]

@craigtaverner I was hoping with this comment from Alex you would catch this one.

I've taken that exact query that Alex added a review to and I simply moved the keep command and placed it between the two lookups (like how he suggested) with this final query:

ROW left = "left", client_ip = "172.21.0.5", message = "Connected to 10.1.0.1", right = "right"
| LOOKUP JOIN clientips_lookup ON client_ip
| KEEP left, client_ip, message, right
| LOOKUP JOIN message_types_lookup ON message

It fails with

path: /_query, params: {format=txt, error_trace=true}, status: 500 java.lang.ArrayIndexOutOfBoundsException: Index -1 out of bounds for length 0
        at org.elasticsearch.compute.operator.lookup.MergePositionsOperator.<init>(MergePositionsOperator.java:76)
        at org.elasticsearch.xpack.esql.enrich.AbstractLookupService.doLookup(AbstractLookupService.java:329)
        at org.elasticsearch.xpack.esql.enrich.AbstractLookupService$TransportHandler.messageReceived(AbstractLookupService.java:447)
        at org.elasticsearch.xpack.esql.enrich.AbstractLookupService$TransportHandler.messageReceived(AbstractLookupService.java:442)
        at org.elasticsearch.server@9.0.0-SNAPSHOT/org.elasticsearch.transport.RequestHandlerRegistry.processMessageReceived(RequestHandlerRegistry.java:90)
        at org.elasticsearch.server@9.0.0-SNAPSHOT/org.elasticsearch.transport.TransportService$6.doRun(TransportService.java:1098)
        at org.elasticsearch.server@9.0.0-SNAPSHOT/org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:1023)
        at org.elasticsearch.server@9.0.0-SNAPSHOT/org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:27)
        at org.elasticsearch.server@9.0.0-SNAPSHOT/org.elasticsearch.common.util.concurrent.TimedRunnable.doRun(TimedRunnable.java:34)
        at org.elasticsearch.server@9.0.0-SNAPSHOT/org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:1023)
        at org.elasticsearch.server@9.0.0-SNAPSHOT/org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:27)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
        at java.base/java.lang.Thread.run(Thread.java:1575)

If it's something easy to fix, do it here, otherwise merge this PR and address this problem ^ in a separate PR right away.

[craigtaverner]

@astefan Your failing query drops all columns from the JOIN, and we do not yet have validation for that case. I would prefer to fix this in another PR, because it is a pre-existing issue, unrelated to multiple joins.

[astefan]

#118778

[astefan]

LGTM

[alex-spies]

Reviewed the changes to LogicalPlanOptimizerTests, these LGTM! Thanks Craig!

[elasticsearchmachine]

💚 Backport successful

Status	Branch	Result
✅	8.x