Skip to content

Commit 6cfb55c

Browse files
julian-elasticjulian-elastic
committed
Refactor to remove UsingJoinType, NaturalJoinType
1 parent fe4c41e commit 6cfb55c

File tree

5 files changed

+19
-122
lines changed

5 files changed

+19
-122
lines changed

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

Lines changed: 8 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,6 @@
128128
import org.elasticsearch.xpack.esql.plan.logical.join.JoinConfig;
129129
import org.elasticsearch.xpack.esql.plan.logical.join.JoinType;
130130
import org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes;
131-
import org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes.UsingJoinType;
132131
import org.elasticsearch.xpack.esql.plan.logical.join.LookupJoin;
133132
import org.elasticsearch.xpack.esql.plan.logical.local.EsqlProject;
134133
import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation;
@@ -775,23 +774,13 @@ private Join resolveLookupJoin(LookupJoin join) {
775774
JoinType type = config.type();
776775

777776
// rewrite the join into an equi-join between the field with the same name between left and right
778-
if (type instanceof UsingJoinType using) {
779-
List<Attribute> cols = using.columns();
777+
if (type == JoinTypes.LEFT) {
780778
// the lookup cannot be resolved, bail out
781-
if (Expressions.anyMatch(cols, c -> c instanceof UnresolvedAttribute ua && ua.customMessage())) {
779+
if (Expressions.anyMatch(join.config().leftFields(), c -> c instanceof UnresolvedAttribute ua && ua.customMessage())) {
782780
return join;
783781
}
784-
785-
JoinType coreJoin = using.coreJoin();
786-
// verify the join type
787-
if (coreJoin != JoinTypes.LEFT) {
788-
String name = cols.get(0).name();
789-
UnresolvedAttribute errorAttribute = new UnresolvedAttribute(
790-
join.source(),
791-
name,
792-
"Only LEFT join is supported with USING"
793-
);
794-
return join.withConfig(new JoinConfig(type, singletonList(errorAttribute), emptyList(), null));
782+
if (Expressions.anyMatch(join.config().rightFields(), c -> c instanceof UnresolvedAttribute ua && ua.customMessage())) {
783+
return join;
795784
}
796785
List<Attribute> leftKeys = new ArrayList<>();
797786
List<Attribute> rightKeys = new ArrayList<>();
@@ -816,22 +805,18 @@ private Join resolveLookupJoin(LookupJoin join) {
816805
}
817806
} else {
818807
// resolve the using columns against the left and the right side then assemble the new join config
819-
leftKeys = resolveUsingColumns(cols, join.left().output(), "left");
820-
rightKeys = resolveUsingColumns(cols, join.right().output(), "right");
808+
leftKeys = resolveUsingColumns(join.config().leftFields(), join.left().output(), "left");
809+
rightKeys = resolveUsingColumns(join.config().rightFields(), join.right().output(), "right");
821810
}
822811

823-
config = new JoinConfig(coreJoin, leftKeys, rightKeys, Predicates.combineAnd(resolvedFilters));
812+
config = new JoinConfig(type, leftKeys, rightKeys, Predicates.combineAnd(resolvedFilters));
824813
return new LookupJoin(join.source(), join.left(), join.right(), config, join.isRemote());
825-
} else if (type != JoinTypes.LEFT) {
814+
} else {
826815
// everything else is unsupported for now
827-
// 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
828-
// more than once.
829816
UnresolvedAttribute errorAttribute = new UnresolvedAttribute(join.source(), "unsupported", "Unsupported join type");
830817
// add error message
831818
return join.withConfig(new JoinConfig(type, singletonList(errorAttribute), emptyList(), null));
832819
}
833-
834-
return join;
835820
}
836821

837822
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
@@ -43,7 +43,6 @@
4343
import org.elasticsearch.xpack.esql.plan.logical.TopN;
4444
import org.elasticsearch.xpack.esql.plan.logical.UnresolvedRelation;
4545
import org.elasticsearch.xpack.esql.plan.logical.inference.Completion;
46-
import org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes;
4746
import org.elasticsearch.xpack.esql.plan.logical.join.LookupJoin;
4847
import org.elasticsearch.xpack.esql.session.EsqlSession.PreAnalysisResult;
4948

@@ -154,9 +153,7 @@ public static PreAnalysisResult resolveFieldNames(LogicalPlan parsed, EnrichReso
154153
enrichRefs.removeIf(attr -> attr instanceof EmptyAttribute);
155154
referencesBuilder.get().addAll(enrichRefs);
156155
} else if (p instanceof LookupJoin join) {
157-
if (join.config().type() instanceof JoinTypes.UsingJoinType usingJoinType) {
158-
joinRefs.addAll(usingJoinType.columns());
159-
}
156+
joinRefs.addAll(join.config().leftFields());
160157
if (keepRefs.isEmpty()) {
161158
// No KEEP commands after the JOIN, so we need to mark this index for "*" field resolution
162159
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;
@@ -3242,12 +3241,11 @@ public void testValidJoinPatternFieldJoin() {
32423241
assertThat(as(join.left(), UnresolvedRelation.class).indexPattern().indexPattern(), equalTo(unquoteIndexPattern(basePattern)));
32433242
assertThat(as(join.right(), UnresolvedRelation.class).indexPattern().indexPattern(), equalTo(unquoteIndexPattern(joinPattern)));
32443243

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

32533251
public void testExpressionJoinNonSnapshotBuild() {
@@ -4604,9 +4602,8 @@ public void testDoubleParamsForIdentifier() {
46044602
LookupJoin join = as(limit.child(), LookupJoin.class);
46054603
UnresolvedRelation ur = as(join.right(), UnresolvedRelation.class);
46064604
assertEquals(ur.indexPattern().indexPattern(), "idx");
4607-
JoinTypes.UsingJoinType joinType = as(join.config().type(), JoinTypes.UsingJoinType.class);
4608-
assertEquals(joinType.coreJoin().joinName(), "LEFT OUTER");
4609-
assertEquals(joinType.columns(), List.of(attribute("f9")));
4605+
assertEquals(join.config().type().joinName(), "LEFT OUTER");
4606+
assertEquals(join.config().leftFields(), List.of(attribute("f9")));
46104607
Rename rename = as(join.left(), Rename.class);
46114608
assertEquals(rename.renamings(), List.of(new Alias(EMPTY, "f.8", attribute("f7*."))));
46124609
Grok grok = as(rename.child(), Grok.class);
@@ -4714,9 +4711,8 @@ public void testDoubleParamsForIdentifier() {
47144711
LookupJoin join = as(limit.child(), LookupJoin.class);
47154712
UnresolvedRelation ur = as(join.right(), UnresolvedRelation.class);
47164713
assertEquals(ur.indexPattern().indexPattern(), "idx");
4717-
JoinTypes.UsingJoinType joinType = as(join.config().type(), JoinTypes.UsingJoinType.class);
4718-
assertEquals(joinType.coreJoin().joinName(), "LEFT OUTER");
4719-
assertEquals(joinType.columns(), List.of(attribute("f13.f14")));
4714+
assertEquals(join.config().type().joinName(), "LEFT OUTER");
4715+
assertEquals(join.config().leftFields(), List.of(attribute("f13.f14")));
47204716
Rename rename = as(join.left(), Rename.class);
47214717
assertEquals(rename.renamings(), List.of(new Alias(EMPTY, "f11*..f.12", attribute("f.9*.f.10."))));
47224718
Grok grok = as(rename.child(), Grok.class);
@@ -4965,9 +4961,8 @@ public void testMixedSingleDoubleParams() {
49654961
LookupJoin join = as(plan, LookupJoin.class);
49664962
UnresolvedRelation ur = as(join.right(), UnresolvedRelation.class);
49674963
assertEquals(ur.indexPattern().indexPattern(), "idx");
4968-
JoinTypes.UsingJoinType joinType = as(join.config().type(), JoinTypes.UsingJoinType.class);
4969-
assertEquals(joinType.coreJoin().joinName(), "LEFT OUTER");
4970-
assertEquals(joinType.columns(), List.of(attribute("f5")));
4964+
assertEquals(join.config().type().joinName(), "LEFT OUTER");
4965+
assertEquals(join.config().leftFields(), List.of(attribute("f5")));
49714966
Drop drop = as(join.left(), Drop.class);
49724967
List<? extends NamedExpression> removals = drop.removals();
49734968
assertEquals(removals.size(), 2);

0 commit comments

Comments
 (0)