Browse Source

ESQL: Fix attribute set equals (#118823) (#119048)

Also add a test that uses this, for lookup join field attribute ids.
Alexander Spies 10 months ago
parent
commit
2ae5911b27

+ 5 - 0
docs/changelog/118823.yaml

@@ -0,0 +1,5 @@
+pr: 118823
+summary: Fix attribute set equals
+area: ES|QL
+type: bug
+issues: []

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

@@ -174,8 +174,12 @@ public class AttributeSet implements Set<Attribute> {
     }
 
     @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

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

@@ -30,7 +30,7 @@ import static org.hamcrest.Matchers.sameInstance;
 
 public class AttributeMapTests extends ESTestCase {
 
-    private static Attribute a(String name) {
+    static Attribute a(String name) {
         return new UnresolvedAttribute(Source.EMPTY, name);
     }
 

+ 42 - 0
x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/AttributeSetTests.java

@@ -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);
+    }
+}

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

@@ -26,6 +26,7 @@ import org.elasticsearch.xpack.esql.VerificationException;
 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;
@@ -2197,6 +2198,36 @@ public class AnalyzerTests extends ESTestCase {
         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());