Skip to content

Commit 1aca0a9

Browse files
julian-elasticjulian-elastic
authored
ESQL: Expression based Lookup Join follow up and refactor (#135225)
Expression based Lookup Join follow up and refactor
1 parent ef4dbe7 commit 1aca0a9

File tree

6 files changed

+32
-139
lines changed

6 files changed

+32
-139
lines changed

‎x-pack/plugin/esql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/esql/EsqlSecurityIT.java‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -828,12 +828,7 @@ private void testLookupJoinFieldLevelSecurityHelper(boolean useExpressionJoin) t
828828
ResponseException error = expectThrows(ResponseException.class, () -> runESQLCommand("fls_user4_1", query));
829829
assertThat(error.getResponse().getStatusLine().getStatusCode(), equalTo(HttpStatus.SC_BAD_REQUEST));
830830
if (useExpressionJoin) {
831-
assertThat(
832-
error.getMessage(),
833-
containsString(
834-
"Join condition must be between one attribute on the left side and one attribute on the right side of the join"
835-
)
836-
);
831+
assertThat(error.getMessage(), containsString("Unsupported join filter expression:value_left == value"));
837832
} else {
838833
assertThat(error.getMessage(), containsString("Unknown column [value] in right side of join"));
839834
}
@@ -907,12 +902,7 @@ private void testLookupJoinFieldLevelSecurityOnAliasHelper(boolean useExpression
907902
ResponseException error = expectThrows(ResponseException.class, () -> runESQLCommand("fls_user4_1_alias", query));
908903
assertThat(error.getResponse().getStatusLine().getStatusCode(), equalTo(HttpStatus.SC_BAD_REQUEST));
909904
if (useExpressionJoin) {
910-
assertThat(
911-
error.getMessage(),
912-
containsString(
913-
"Join condition must be between one attribute on the left side and one attribute on the right side of the join"
914-
)
915-
);
905+
assertThat(error.getMessage(), containsString("Unsupported join filter expression:value_left == value"));
916906
} else {
917907
assertThat(error.getMessage(), containsString("Unknown column [value] in right side of join"));
918908
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java‎

Lines changed: 19 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,6 @@
130130
import org.elasticsearch.xpack.esql.plan.logical.join.JoinConfig;
131131
import org.elasticsearch.xpack.esql.plan.logical.join.JoinType;
132132
import org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes;
133-
import org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes.UsingJoinType;
134133
import org.elasticsearch.xpack.esql.plan.logical.join.LookupJoin;
135134
import org.elasticsearch.xpack.esql.plan.logical.local.EsqlProject;
136135
import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation;
@@ -760,9 +759,9 @@ private Expression resolveAndOrientJoinCondition(Expression condition, Attribute
760759
if (leftIsFromRight && rightIsFromLeft) {
761760
return comp.swapLeftAndRight(); // Swapped orientation
762761
}
763-
764-
// Invalid orientation (e.g., both from left or both from right)
765-
throw new IllegalArgumentException(
762+
return new UnresolvedAttribute(
763+
condition.source(),
764+
"unsupported",
766765
"Join condition must be between one attribute on the left side and "
767766
+ "one attribute on the right side of the join, but found: "
768767
+ condition.sourceText()
@@ -777,24 +776,14 @@ private Join resolveLookupJoin(LookupJoin join) {
777776
JoinType type = config.type();
778777

779778
// rewrite the join into an equi-join between the field with the same name between left and right
780-
if (type instanceof UsingJoinType using) {
781-
List<Attribute> cols = using.columns();
779+
if (type == JoinTypes.LEFT) {
782780
// the lookup cannot be resolved, bail out
783-
if (Expressions.anyMatch(cols, c -> c instanceof UnresolvedAttribute ua && ua.customMessage())) {
781+
if (Expressions.anyMatch(
782+
join.references().stream().toList(),
783+
c -> c instanceof UnresolvedAttribute ua && ua.customMessage()
784+
)) {
784785
return join;
785786
}
786-
787-
JoinType coreJoin = using.coreJoin();
788-
// verify the join type
789-
if (coreJoin != JoinTypes.LEFT) {
790-
String name = cols.get(0).name();
791-
UnresolvedAttribute errorAttribute = new UnresolvedAttribute(
792-
join.source(),
793-
name,
794-
"Only LEFT join is supported with USING"
795-
);
796-
return join.withConfig(new JoinConfig(type, singletonList(errorAttribute), emptyList(), null));
797-
}
798787
List<Attribute> leftKeys = new ArrayList<>();
799788
List<Attribute> rightKeys = new ArrayList<>();
800789
List<Expression> resolvedFilters = new ArrayList<>();
@@ -813,27 +802,29 @@ private Join resolveLookupJoin(LookupJoin join) {
813802
leftKeys.add(leftAttribute);
814803
rightKeys.add(rightAttribute);
815804
} else {
816-
throw new IllegalArgumentException("Unsupported join filter expression: " + expression);
805+
UnresolvedAttribute errorAttribute = new UnresolvedAttribute(
806+
expression.source(),
807+
"unsupported",
808+
"Unsupported join filter expression:" + expression.sourceText()
809+
);
810+
return join.withConfig(new JoinConfig(type, singletonList(errorAttribute), emptyList(), null));
811+
817812
}
818813
}
819814
} else {
820815
// resolve the using columns against the left and the right side then assemble the new join config
821-
leftKeys = resolveUsingColumns(cols, join.left().output(), "left");
822-
rightKeys = resolveUsingColumns(cols, join.right().output(), "right");
816+
leftKeys = resolveUsingColumns(join.config().leftFields(), join.left().output(), "left");
817+
rightKeys = resolveUsingColumns(join.config().rightFields(), join.right().output(), "right");
823818
}
824819

825-
config = new JoinConfig(coreJoin, leftKeys, rightKeys, Predicates.combineAnd(resolvedFilters));
820+
config = new JoinConfig(type, leftKeys, rightKeys, Predicates.combineAnd(resolvedFilters));
826821
return new LookupJoin(join.source(), join.left(), join.right(), config, join.isRemote());
827-
} else if (type != JoinTypes.LEFT) {
822+
} else {
828823
// everything else is unsupported for now
829-
// LEFT can only happen by being mapped from a USING above. So we need to exclude this as well because this rule can be run
830-
// more than once.
831824
UnresolvedAttribute errorAttribute = new UnresolvedAttribute(join.source(), "unsupported", "Unsupported join type");
832825
// add error message
833826
return join.withConfig(new JoinConfig(type, singletonList(errorAttribute), emptyList(), null));
834827
}
835-
836-
return join;
837828
}
838829

839830
private LogicalPlan resolveFork(Fork fork, AnalyzerContext context) {

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/JoinTypes.java‎

Lines changed: 0 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,9 @@
99
import org.elasticsearch.common.io.stream.StreamInput;
1010
import org.elasticsearch.common.io.stream.StreamOutput;
1111
import org.elasticsearch.common.util.Maps;
12-
import org.elasticsearch.xpack.esql.core.capabilities.Resolvables;
13-
import org.elasticsearch.xpack.esql.core.expression.Attribute;
1412

1513
import java.io.IOException;
16-
import java.util.List;
1714
import java.util.Map;
18-
import java.util.Objects;
1915

2016
/**
2117
* Utility class defining the concrete types of joins supported by ESQL.
@@ -69,80 +65,6 @@ public void writeTo(StreamOutput out) throws IOException {
6965
}
7066
}
7167

72-
/**
73-
* Join type for the USING clause - shorthand for defining an equi-join (equality join meaning the condition checks if columns across
74-
* each side of the join are equal).
75-
* One important difference is that the USING clause returns the join column only once, at the beginning of the result set.
76-
*/
77-
public static class UsingJoinType implements JoinType {
78-
private final List<Attribute> columns;
79-
private final JoinType coreJoin;
80-
81-
public UsingJoinType(JoinType coreJoin, List<Attribute> columns) {
82-
this.columns = columns;
83-
this.coreJoin = coreJoin;
84-
}
85-
86-
@Override
87-
public String joinName() {
88-
return coreJoin.joinName() + " USING " + columns.toString();
89-
}
90-
91-
@Override
92-
public void writeTo(StreamOutput out) throws IOException {
93-
throw new IllegalArgumentException("USING join type should not be serialized due to being rewritten");
94-
}
95-
96-
public JoinType coreJoin() {
97-
return coreJoin;
98-
}
99-
100-
public List<Attribute> columns() {
101-
return columns;
102-
}
103-
104-
@Override
105-
public boolean resolved() {
106-
return Resolvables.resolved(columns);
107-
}
108-
109-
@Override
110-
public int hashCode() {
111-
return Objects.hash(columns, coreJoin);
112-
}
113-
114-
@Override
115-
public boolean equals(Object o) {
116-
if (this == o) return true;
117-
if (o == null || getClass() != o.getClass()) return false;
118-
UsingJoinType that = (UsingJoinType) o;
119-
return Objects.equals(columns, that.columns) && coreJoin == that.coreJoin;
120-
}
121-
122-
@Override
123-
public String toString() {
124-
return joinName();
125-
}
126-
}
127-
128-
/**
129-
* Private class so it doesn't get used yet it is defined to showcase why the join type was defined as an interface instead of a simpler
130-
* enum.
131-
*/
132-
private abstract static class NaturalJoinType implements JoinType {
133-
134-
private final JoinType joinType;
135-
136-
private NaturalJoinType(JoinType joinType) {
137-
this.joinType = joinType;
138-
}
139-
140-
@Override
141-
public String joinName() {
142-
return "NATURAL " + joinType.joinName();
143-
}
144-
}
145-
14668
public static JoinType readFrom(StreamInput in) throws IOException {
14769
byte id = in.readByte();
14870
JoinType type = JOIN_TYPES.get(id);

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/LookupJoin.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,9 @@
1818
import org.elasticsearch.xpack.esql.plan.logical.Limit;
1919
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
2020
import org.elasticsearch.xpack.esql.plan.logical.SurrogateLogicalPlan;
21-
import org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes.UsingJoinType;
2221

2322
import java.util.List;
2423

25-
import static java.util.Collections.emptyList;
2624
import static org.elasticsearch.xpack.esql.common.Failure.fail;
2725
import static org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes.LEFT;
2826

@@ -40,7 +38,7 @@ public LookupJoin(
4038
boolean isRemote,
4139
@Nullable Expression joinOnConditions
4240
) {
43-
this(source, left, right, new UsingJoinType(LEFT, joinFields), emptyList(), emptyList(), isRemote, joinOnConditions);
41+
this(source, left, right, LEFT, joinFields, joinFields, isRemote, joinOnConditions);
4442
}
4543

4644
public LookupJoin(

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

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@
4141
import org.elasticsearch.xpack.esql.plan.logical.TopN;
4242
import org.elasticsearch.xpack.esql.plan.logical.UnresolvedRelation;
4343
import org.elasticsearch.xpack.esql.plan.logical.inference.Completion;
44-
import org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes;
4544
import org.elasticsearch.xpack.esql.plan.logical.join.LookupJoin;
4645
import org.elasticsearch.xpack.esql.session.EsqlSession.PreAnalysisResult;
4746

@@ -147,9 +146,7 @@ public static PreAnalysisResult resolveFieldNames(LogicalPlan parsed, boolean ha
147146
enrichRefs.removeIf(attr -> attr instanceof EmptyAttribute);
148147
referencesBuilder.get().addAll(enrichRefs);
149148
} else if (p instanceof LookupJoin join) {
150-
if (join.config().type() instanceof JoinTypes.UsingJoinType usingJoinType) {
151-
joinRefs.addAll(usingJoinType.columns());
152-
}
149+
joinRefs.addAll(join.config().leftFields());
153150
if (keepRefs.isEmpty()) {
154151
// No KEEP commands after the JOIN, so we need to mark this index for "*" field resolution
155152
wildcardJoinIndices.add(((UnresolvedRelation) join.right()).indexPattern().indexPattern());

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,6 @@
7272
import org.elasticsearch.xpack.esql.plan.logical.fuse.Fuse;
7373
import org.elasticsearch.xpack.esql.plan.logical.inference.Completion;
7474
import org.elasticsearch.xpack.esql.plan.logical.inference.Rerank;
75-
import org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes;
7675
import org.elasticsearch.xpack.esql.plan.logical.join.LookupJoin;
7776

7877
import java.util.ArrayList;
@@ -3240,12 +3239,11 @@ public void testValidJoinPatternFieldJoin() {
32403239
assertThat(as(join.left(), UnresolvedRelation.class).indexPattern().indexPattern(), equalTo(unquoteIndexPattern(basePattern)));
32413240
assertThat(as(join.right(), UnresolvedRelation.class).indexPattern().indexPattern(), equalTo(unquoteIndexPattern(joinPattern)));
32423241

3243-
var joinType = as(join.config().type(), JoinTypes.UsingJoinType.class);
3244-
assertThat(joinType.columns(), hasSize(numberOfOnFields));
3242+
assertThat(join.config().leftFields(), hasSize(numberOfOnFields));
32453243
for (int i = 0; i < numberOfOnFields; i++) {
3246-
assertThat(as(joinType.columns().get(i), UnresolvedAttribute.class).name(), equalTo(existingIdentifiers.get(i)));
3244+
assertThat(as(join.config().leftFields().get(i), UnresolvedAttribute.class).name(), equalTo(existingIdentifiers.get(i)));
32473245
}
3248-
assertThat(joinType.coreJoin().joinName(), equalTo("LEFT OUTER"));
3246+
assertThat(join.config().type().joinName(), equalTo("LEFT OUTER"));
32493247
}
32503248

32513249
/**
@@ -4595,9 +4593,8 @@ public void testDoubleParamsForIdentifier() {
45954593
LookupJoin join = as(limit.child(), LookupJoin.class);
45964594
UnresolvedRelation ur = as(join.right(), UnresolvedRelation.class);
45974595
assertEquals(ur.indexPattern().indexPattern(), "idx");
4598-
JoinTypes.UsingJoinType joinType = as(join.config().type(), JoinTypes.UsingJoinType.class);
4599-
assertEquals(joinType.coreJoin().joinName(), "LEFT OUTER");
4600-
assertEquals(joinType.columns(), List.of(attribute("f9")));
4596+
assertEquals(join.config().type().joinName(), "LEFT OUTER");
4597+
assertEquals(join.config().leftFields(), List.of(attribute("f9")));
46014598
Rename rename = as(join.left(), Rename.class);
46024599
assertEquals(rename.renamings(), List.of(new Alias(EMPTY, "f.8", attribute("f7*."))));
46034600
Grok grok = as(rename.child(), Grok.class);
@@ -4705,9 +4702,8 @@ public void testDoubleParamsForIdentifier() {
47054702
LookupJoin join = as(limit.child(), LookupJoin.class);
47064703
UnresolvedRelation ur = as(join.right(), UnresolvedRelation.class);
47074704
assertEquals(ur.indexPattern().indexPattern(), "idx");
4708-
JoinTypes.UsingJoinType joinType = as(join.config().type(), JoinTypes.UsingJoinType.class);
4709-
assertEquals(joinType.coreJoin().joinName(), "LEFT OUTER");
4710-
assertEquals(joinType.columns(), List.of(attribute("f13.f14")));
4705+
assertEquals(join.config().type().joinName(), "LEFT OUTER");
4706+
assertEquals(join.config().leftFields(), List.of(attribute("f13.f14")));
47114707
Rename rename = as(join.left(), Rename.class);
47124708
assertEquals(rename.renamings(), List.of(new Alias(EMPTY, "f11*..f.12", attribute("f.9*.f.10."))));
47134709
Grok grok = as(rename.child(), Grok.class);
@@ -4956,9 +4952,8 @@ public void testMixedSingleDoubleParams() {
49564952
LookupJoin join = as(plan, LookupJoin.class);
49574953
UnresolvedRelation ur = as(join.right(), UnresolvedRelation.class);
49584954
assertEquals(ur.indexPattern().indexPattern(), "idx");
4959-
JoinTypes.UsingJoinType joinType = as(join.config().type(), JoinTypes.UsingJoinType.class);
4960-
assertEquals(joinType.coreJoin().joinName(), "LEFT OUTER");
4961-
assertEquals(joinType.columns(), List.of(attribute("f5")));
4955+
assertEquals(join.config().type().joinName(), "LEFT OUTER");
4956+
assertEquals(join.config().leftFields(), List.of(attribute("f5")));
49624957
Drop drop = as(join.left(), Drop.class);
49634958
List<? extends NamedExpression> removals = drop.removals();
49644959
assertEquals(removals.size(), 2);

0 commit comments

Comments
 (0)