Skip to content

Commit

Permalink
ESQL: Fix attribute set equals (elastic#118823) (elastic#119048)
Browse files Browse the repository at this point in the history
Also add a test that uses this, for lookup join field attribute ids.
  • Loading branch information
alex-spies authored Dec 19, 2024
1 parent 023f73f commit 2ae5911
Show file tree
Hide file tree
Showing 5 changed files with 85 additions and 3 deletions.
5 changes: 5 additions & 0 deletions docs/changelog/118823.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 118823
summary: Fix attribute set equals
area: ES|QL
type: bug
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,12 @@ public Stream<Attribute> parallelStream() {
}

@Override
public boolean equals(Object o) {
return delegate.equals(o);
public boolean equals(Object obj) {
if (obj instanceof AttributeSet as) {
obj = as.delegate;
}

return delegate.equals(obj);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@

public class AttributeMapTests extends ESTestCase {

private static Attribute a(String name) {
static Attribute a(String name) {
return new UnresolvedAttribute(Source.EMPTY, name);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
package org.elasticsearch.xpack.esql.core.expression;

import org.elasticsearch.test.ESTestCase;

import java.util.List;

import static org.elasticsearch.xpack.esql.core.expression.AttributeMapTests.a;

public class AttributeSetTests extends ESTestCase {

public void testEquals() {
Attribute a1 = a("1");
Attribute a2 = a("2");

AttributeSet first = new AttributeSet(List.of(a1, a2));
assertEquals(first, first);

AttributeSet second = new AttributeSet();
second.add(a1);
second.add(a2);

assertEquals(first, second);
assertEquals(second, first);

AttributeSet third = new AttributeSet();
third.add(a("1"));
third.add(a("2"));

assertNotEquals(first, third);
assertNotEquals(third, first);

assertEquals(AttributeSet.EMPTY, AttributeSet.EMPTY);
assertEquals(AttributeSet.EMPTY, first.intersect(third));
assertEquals(third.intersect(first), AttributeSet.EMPTY);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.elasticsearch.xpack.esql.action.EsqlCapabilities;
import org.elasticsearch.xpack.esql.core.expression.Alias;
import org.elasticsearch.xpack.esql.core.expression.Attribute;
import org.elasticsearch.xpack.esql.core.expression.AttributeSet;
import org.elasticsearch.xpack.esql.core.expression.Expressions;
import org.elasticsearch.xpack.esql.core.expression.FieldAttribute;
import org.elasticsearch.xpack.esql.core.expression.Literal;
Expand Down Expand Up @@ -2197,6 +2198,36 @@ public void testLookupJoinUnknownField() {
assertThat(e.getMessage(), containsString(errorMessage3 + "right side of join"));
}

public void testMultipleLookupJoinsGiveDifferentAttributes() {
assumeTrue("requires LOOKUP JOIN capability", EsqlCapabilities.Cap.JOIN_LOOKUP_V8.isEnabled());

// The field attributes that get contributed by different LOOKUP JOIN commands must have different name ids,
// even if they have the same names. Otherwise, things like dependency analysis - like in PruneColumns - cannot work based on
// name ids and shadowing semantics proliferate into all kinds of optimizer code.

String query = "FROM test"
+ "| EVAL language_code = languages"
+ "| LOOKUP JOIN languages_lookup ON language_code"
+ "| LOOKUP JOIN languages_lookup ON language_code";
LogicalPlan analyzedPlan = analyze(query);

List<AttributeSet> lookupFields = new ArrayList<>();
List<Set<String>> lookupFieldNames = new ArrayList<>();
analyzedPlan.forEachUp(EsRelation.class, esRelation -> {
if (esRelation.indexMode() == IndexMode.LOOKUP) {
lookupFields.add(esRelation.outputSet());
lookupFieldNames.add(esRelation.outputSet().stream().map(NamedExpression::name).collect(Collectors.toSet()));
}
});

assertEquals(lookupFieldNames.size(), 2);
assertEquals(lookupFieldNames.get(0), lookupFieldNames.get(1));

assertEquals(lookupFields.size(), 2);
AttributeSet intersection = lookupFields.get(0).intersect(lookupFields.get(1));
assertEquals(AttributeSet.EMPTY, intersection);
}

public void testLookupJoinIndexMode() {
assumeTrue("requires LOOKUP JOIN capability", EsqlCapabilities.Cap.JOIN_LOOKUP_V8.isEnabled());

Expand Down

0 comments on commit 2ae5911

Please sign in to comment.