Skip to content

Commit 9cc6cd4

Browse files
authored
ESQL: Fix attribute set equals (#118823)
Also add a test that uses this, for lookup join field attribute ids.
1 parent 90f038d commit 9cc6cd4

File tree

5 files changed

+85
-3
lines changed

5 files changed

+85
-3
lines changed

docs/changelog/118823.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 118823
2+
summary: Fix attribute set equals
3+
area: ES|QL
4+
type: bug
5+
issues: []

x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/AttributeSet.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -174,8 +174,12 @@ public Stream<Attribute> parallelStream() {
174174
}
175175

176176
@Override
177-
public boolean equals(Object o) {
178-
return delegate.equals(o);
177+
public boolean equals(Object obj) {
178+
if (obj instanceof AttributeSet as) {
179+
obj = as.delegate;
180+
}
181+
182+
return delegate.equals(obj);
179183
}
180184

181185
@Override

x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/AttributeMapTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030

3131
public class AttributeMapTests extends ESTestCase {
3232

33-
private static Attribute a(String name) {
33+
static Attribute a(String name) {
3434
return new UnresolvedAttribute(Source.EMPTY, name);
3535
}
3636

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
package org.elasticsearch.xpack.esql.core.expression;
8+
9+
import org.elasticsearch.test.ESTestCase;
10+
11+
import java.util.List;
12+
13+
import static org.elasticsearch.xpack.esql.core.expression.AttributeMapTests.a;
14+
15+
public class AttributeSetTests extends ESTestCase {
16+
17+
public void testEquals() {
18+
Attribute a1 = a("1");
19+
Attribute a2 = a("2");
20+
21+
AttributeSet first = new AttributeSet(List.of(a1, a2));
22+
assertEquals(first, first);
23+
24+
AttributeSet second = new AttributeSet();
25+
second.add(a1);
26+
second.add(a2);
27+
28+
assertEquals(first, second);
29+
assertEquals(second, first);
30+
31+
AttributeSet third = new AttributeSet();
32+
third.add(a("1"));
33+
third.add(a("2"));
34+
35+
assertNotEquals(first, third);
36+
assertNotEquals(third, first);
37+
38+
assertEquals(AttributeSet.EMPTY, AttributeSet.EMPTY);
39+
assertEquals(AttributeSet.EMPTY, first.intersect(third));
40+
assertEquals(third.intersect(first), AttributeSet.EMPTY);
41+
}
42+
}

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import org.elasticsearch.xpack.esql.action.EsqlCapabilities;
2121
import org.elasticsearch.xpack.esql.core.expression.Alias;
2222
import org.elasticsearch.xpack.esql.core.expression.Attribute;
23+
import org.elasticsearch.xpack.esql.core.expression.AttributeSet;
2324
import org.elasticsearch.xpack.esql.core.expression.Expressions;
2425
import org.elasticsearch.xpack.esql.core.expression.FieldAttribute;
2526
import org.elasticsearch.xpack.esql.core.expression.Literal;
@@ -2190,6 +2191,36 @@ public void testLookupJoinUnknownField() {
21902191
assertThat(e.getMessage(), containsString(errorMessage3 + "right side of join"));
21912192
}
21922193

2194+
public void testMultipleLookupJoinsGiveDifferentAttributes() {
2195+
assumeTrue("requires LOOKUP JOIN capability", EsqlCapabilities.Cap.JOIN_LOOKUP_V8.isEnabled());
2196+
2197+
// The field attributes that get contributed by different LOOKUP JOIN commands must have different name ids,
2198+
// even if they have the same names. Otherwise, things like dependency analysis - like in PruneColumns - cannot work based on
2199+
// name ids and shadowing semantics proliferate into all kinds of optimizer code.
2200+
2201+
String query = "FROM test"
2202+
+ "| EVAL language_code = languages"
2203+
+ "| LOOKUP JOIN languages_lookup ON language_code"
2204+
+ "| LOOKUP JOIN languages_lookup ON language_code";
2205+
LogicalPlan analyzedPlan = analyze(query);
2206+
2207+
List<AttributeSet> lookupFields = new ArrayList<>();
2208+
List<Set<String>> lookupFieldNames = new ArrayList<>();
2209+
analyzedPlan.forEachUp(EsRelation.class, esRelation -> {
2210+
if (esRelation.indexMode() == IndexMode.LOOKUP) {
2211+
lookupFields.add(esRelation.outputSet());
2212+
lookupFieldNames.add(esRelation.outputSet().stream().map(NamedExpression::name).collect(Collectors.toSet()));
2213+
}
2214+
});
2215+
2216+
assertEquals(lookupFieldNames.size(), 2);
2217+
assertEquals(lookupFieldNames.get(0), lookupFieldNames.get(1));
2218+
2219+
assertEquals(lookupFields.size(), 2);
2220+
AttributeSet intersection = lookupFields.get(0).intersect(lookupFields.get(1));
2221+
assertEquals(AttributeSet.EMPTY, intersection);
2222+
}
2223+
21932224
public void testLookupJoinIndexMode() {
21942225
assumeTrue("requires LOOKUP JOIN capability", EsqlCapabilities.Cap.JOIN_LOOKUP_V8.isEnabled());
21952226

0 commit comments

Comments
 (0)