Skip to content

Lookup join on multiple join fields not yet supported #118858

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 18, 2024
Merged

Lookup join on multiple join fields not yet supported #118858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2e5622d
Tests
astefan Dec 17, 2024
942d0ee
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Dec 17, 2024
fb6c69e
More tests and capabilities
astefan Dec 17, 2024
7d3b47c
spotless
astefan Dec 17, 2024
804833f
Update docs/changelog/118858.yaml
astefan Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/118858.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 118858
summary: Lookup join on multiple join fields not yet supported
area: ES|QL
type: enhancement
issues: []
5 changes: 5 additions & 0 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,11 @@ public PlanFactory visitJoinCommand(EsqlBaseParser.JoinCommandContext ctx) {
}
}

var matchFieldsCount = joinFields.size();
if (matchFieldsCount > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check (above) that a same field isn't already added? Specifying a join key multiple times doesn't actually modify the output, so we could allow that (like we do for groups now, for instance, or drop or keep).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean lookup join test on languages, languages should be considered as lookup join test on languages?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was what I was thinking. Or, when we might support more, ON x, y, x be same as ON x, y.
But can be left as is as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo, we are hiding a behavior (eliminating duplicates) without the user being notified of, what is in essence, an incorrect query. I think for now I will leave this as is in this PR and relax things later, if we identify this as a drawback. Thanks.

throw new ParsingException(source, "JOIN ON clause only supports one field at the moment, found [{}]", matchFieldsCount);
}

return p -> new LookupJoin(source, p, right, joinFields);
}
}
41 changes: 41 additions & 0 deletions x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/ParsingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.json.JsonXContent;
import org.elasticsearch.xpack.esql.LoadMapping;
import org.elasticsearch.xpack.esql.action.EsqlCapabilities;
import org.elasticsearch.xpack.esql.core.expression.Expression;
import org.elasticsearch.xpack.esql.core.type.DataType;
import org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry;
Expand Down Expand Up @@ -111,6 +112,46 @@ public void testTooBigQuery() {
assertEquals("-1:-1: ESQL statement is too large [1000011 characters > 1000000]", error(query.toString()));
}

public void testJoinOnConstant() {
assumeTrue("LOOKUP JOIN available as snapshot only", EsqlCapabilities.Cap.JOIN_LOOKUP_V7.isEnabled());
assertEquals(
"1:55: JOIN ON clause only supports fields at the moment, found [123]",
error("row languages = 1, gender = \"f\" | lookup join test on 123")
);
assertEquals(
"1:55: JOIN ON clause only supports fields at the moment, found [\"abc\"]",
error("row languages = 1, gender = \"f\" | lookup join test on \"abc\"")
);
assertEquals(
"1:55: JOIN ON clause only supports fields at the moment, found [false]",
error("row languages = 1, gender = \"f\" | lookup join test on false")
);
}

public void testJoinOnMultipleFields() {
assumeTrue("LOOKUP JOIN available as snapshot only", EsqlCapabilities.Cap.JOIN_LOOKUP_V7.isEnabled());
assertEquals(
"1:35: JOIN ON clause only supports one field at the moment, found [2]",
error("row languages = 1, gender = \"f\" | lookup join test on gender, languages")
);
}

public void testJoinTwiceOnTheSameField() {
assumeTrue("LOOKUP JOIN available as snapshot only", EsqlCapabilities.Cap.JOIN_LOOKUP_V7.isEnabled());
assertEquals(
"1:35: JOIN ON clause only supports one field at the moment, found [2]",
error("row languages = 1, gender = \"f\" | lookup join test on languages, languages")
);
}

public void testJoinTwiceOnTheSameField_TwoLookups() {
assumeTrue("LOOKUP JOIN available as snapshot only", EsqlCapabilities.Cap.JOIN_LOOKUP_V7.isEnabled());
assertEquals(
"1:80: JOIN ON clause only supports one field at the moment, found [2]",
error("row languages = 1, gender = \"f\" | lookup join test on languages | eval x = 1 | lookup join test on gender, gender")
);
}

private String functionName(EsqlFunctionRegistry registry, Expression functionCall) {
for (FunctionDefinition def : registry.listFunctions()) {
if (functionCall.getClass().equals(def.clazz())) {
Expand Down