ESQL: Make equals include ids for Alias, TypedAttribute

docs/changelog/132455.yaml

@@ -0,0 +1,7 @@
+pr: 132455
+summary: "Make equals include ids for Alias, `TypedAttribute`"
+area: ES|QL
+type: bug
+issues:
+ - 131509
+ - 132634

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

@@ -22,11 +22,12 @@
 
 /**
  * An {@code Alias} is a {@code NamedExpression} that gets renamed to something else through the Alias.
- *
+ * <p>
  * For example, in the statement {@code 5 + 2 AS x}, {@code x} is an alias which is points to {@code ADD(5, 2)}.
- *
  * And in {@code SELECT col AS x} "col" is a named expression that gets renamed to "x" through an alias.
- *
+ * <p>
+ * Note on equality: The id is respected in {@link #equals(Object)} for Alias because the {@link ReferenceAttribute}
+ * created by {@link #toAttribute()} uses it.
  */
 public final class Alias extends NamedExpression {
     public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(NamedExpression.class, "Alias", Alias::new);
@@ -71,6 +72,10 @@ public Alias(StreamInput in) throws IOException {
         );
     }
 
+    public Alias withId(NameId id) {
+        return new Alias(source(), name(), child(), id, synthetic());
+    }
+
     @Override
     public void writeTo(StreamOutput out) throws IOException {
         Source.EMPTY.writeTo(out);

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

@@ -22,16 +22,55 @@
 /**
  * {@link Expression}s that can be materialized and describe properties of the derived table.
  * In other words, an attribute represent a column in the results of a query.
- *
+ * <p>
  * In the statement {@code SELECT ABS(foo), A, B+C FROM ...} the three named
  * expressions {@code ABS(foo), A, B+C} get converted to attributes and the user can
  * only see Attributes.
- *
+ * <p>
  * In the statement {@code SELECT foo FROM TABLE WHERE foo > 10 + 1} only {@code foo} inside the SELECT
  * is a named expression (an {@code Alias} will be created automatically for it).
  * The rest are not as they are not part of the projection and thus are not part of the derived table.
+ * <p>
+ * Note on equality: Because the name alone is not sufficient to identify an attribute
+ * (two different relations can have the same attribute name), attributes get a unique {@link #id()}
+ * assigned at creation which is respected in equality checks and hashing.
+ * <p>
+ * Caution! {@link #semanticEquals(Expression)} and {@link #semanticHash()} rely solely on the id.
+ * But this doesn't extend to expressions containing attributes as children.
  */
 public abstract class Attribute extends NamedExpression {
+    /**
+     * A wrapper class where equality of the contained attribute ignores the {@link Attribute#id()}. Useful when we want to create new
+     * attributes and want to avoid duplicates. Because we assign unique ids on creation, a normal equality check would always fail when
+     * we create the "same" attribute again.
+     * <p>
+     * C.f. {@link AttributeMap} (and its contained wrapper class) which does the exact opposite: ignores everything but the id by
+     * using {@link Attribute#semanticEquals(Expression)}.
+     */
+    public record IdIgnoringWrapper(Attribute inner) {
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) {
+                return true;
+            }
+            if (o == null || getClass() != o.getClass()) {
+                return false;
+            }
+
+            Attribute otherAttribute = ((IdIgnoringWrapper) o).inner();
+            return inner().equals(otherAttribute, true);
+        }
+
+        @Override
+        public int hashCode() {
+            return inner().hashCode(true);
+        }
+    }
+
+    public IdIgnoringWrapper ignoreId() {
+        return new IdIgnoringWrapper(this);
+    }
+
     /**
      * Changing this will break bwc with 8.15, see {@link FieldAttribute#fieldName()}.
      */
@@ -152,7 +191,7 @@ public int semanticHash() {
 
     @Override
     public boolean semanticEquals(Expression other) {
-        return other instanceof Attribute ? id().equals(((Attribute) other).id()) : false;
+        return other instanceof Attribute otherAttr && id().equals(otherAttr.id());
     }
 
     @Override
@@ -161,15 +200,16 @@ protected Expression canonicalize() {
     }
 
     @Override
-    @SuppressWarnings("checkstyle:EqualsHashCode")// equals is implemented in parent. See innerEquals instead
-    public int hashCode() {
-        return Objects.hash(super.hashCode(), qualifier, nullability);
+    protected int innerHashCode(boolean ignoreIds) {
+        return Objects.hash(super.innerHashCode(ignoreIds), qualifier, nullability);
     }
 
     @Override
-    protected boolean innerEquals(Object o) {
+    protected boolean innerEquals(Object o, boolean ignoreIds) {
         var other = (Attribute) o;
-        return super.innerEquals(other) && Objects.equals(qualifier, other.qualifier) && Objects.equals(nullability, other.nullability);
+        return super.innerEquals(other, ignoreIds)
+            && Objects.equals(qualifier, other.qualifier)
+            && Objects.equals(nullability, other.nullability);
     }
 
     @Override

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

@@ -17,9 +17,10 @@
 
 /**
  * Marker for optional attributes. Acting as a dummy placeholder to avoid using null
- * in the tree (which is not allowed).
+ * in the tree (which is not allowed). All empty attributes are considered equal.
  */
 public class EmptyAttribute extends Attribute {
+    // TODO: Could be a singleton - all instances are already considered equal.
     public EmptyAttribute(Source source) {
         super(source, StringUtils.EMPTY, null);
     }
@@ -78,13 +79,22 @@ protected NodeInfo<? extends Expression> info() {
     }
 
     @Override
-    @SuppressWarnings("checkstyle:EqualsHashCode")// equals is implemented in parent. See innerEquals instead
-    public int hashCode() {
+    protected int innerHashCode(boolean ignoreIds) {
         return EmptyAttribute.class.hashCode();
     }
 
     @Override
-    protected boolean innerEquals(Object o) {
+    protected boolean innerEquals(Object o, boolean ignoreIds) {
         return true;
     }
+
+    @Override
+    public int semanticHash() {
+        return EmptyAttribute.class.hashCode();
+    }
+
+    @Override
+    public boolean semanticEquals(Expression other) {
+        return other instanceof EmptyAttribute;
+    }
 }

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

@@ -180,10 +180,26 @@ protected Expression canonicalize() {
         return replaceChildrenSameSize(canonicalChildren);
     }
 
+    /**
+     * Whether this expression means the same as {@code other}, even if they are not exactly equal.
+     * For example, {@code a + b} and {@code b + a} are not equal, but they are semantically equal.
+     * <p>
+     * If two expressions are equal, they are also semantically equal, but the reverse is generally not true.
+     * <p>
+     * Caution! {@link Attribute#semanticEquals(Expression)} is especially lenient, as it considers two attributes
+     * with the same {@link NameId} to be semantically equal, even if they have different data types or are represented using different
+     * classes.
+     * <p>
+     * But this doesn't extend to expressions containing attributes as children, which is pretty inconsistent.
+     * We have to revisit this before using {@link #semanticEquals} in more places.
+     */
     public boolean semanticEquals(Expression other) {
         return canonical().equals(other.canonical());
     }
 
+    /**
+     * A hash code that is consistent with {@link #semanticEquals}.
+     */
     public int semanticHash() {
         return canonical().hashCode();
     }

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

@@ -231,15 +231,14 @@ public Attribute withDataType(DataType type) {
     }
 
     @Override
-    @SuppressWarnings("checkstyle:EqualsHashCode")// equals is implemented in parent. See innerEquals instead
-    public int hashCode() {
-        return Objects.hash(super.hashCode(), parentName, field);
+    protected int innerHashCode(boolean ignoreIds) {
+        return Objects.hash(super.innerHashCode(ignoreIds), parentName, field);
     }
 
     @Override
-    protected boolean innerEquals(Object o) {
+    protected boolean innerEquals(Object o, boolean ignoreIds) {
         var other = (FieldAttribute) o;
-        return super.innerEquals(other) && Objects.equals(parentName, other.parentName) && Objects.equals(field, other.field);
+        return super.innerEquals(other, ignoreIds) && Objects.equals(parentName, other.parentName) && Objects.equals(field, other.field);
     }
 
     @Override

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

@@ -170,14 +170,13 @@ public static boolean isScoreAttribute(Expression a) {
     }
 
     @Override
-    @SuppressWarnings("checkstyle:EqualsHashCode")// equals is implemented in parent. See innerEquals instead
-    public int hashCode() {
-        return Objects.hash(super.hashCode(), searchable);
+    protected int innerHashCode(boolean ignoreIds) {
+        return Objects.hash(super.innerHashCode(ignoreIds), searchable);
     }
 
     @Override
-    protected boolean innerEquals(Object o) {
+    protected boolean innerEquals(Object o, boolean ignoreIds) {
         var other = (MetadataAttribute) o;
-        return super.innerEquals(other) && searchable == other.searchable;
+        return super.innerEquals(other, ignoreIds) && searchable == other.searchable;
     }
 }

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

@@ -31,6 +31,15 @@ public NameId() {
         this.id = COUNTER.incrementAndGet();
     }
 
+    /**
+     * Absolutely only intended for tests, esp. to deal with serialization. Never use in production as it breaks the
+     * uniqueness guarantee.
+     */
+    @Deprecated
+    public NameId(long id) {
+        this.id = id;
+    }
+
     @Override
     public int hashCode() {
         return Long.hashCode(id);

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

@@ -27,6 +27,9 @@ public NamedExpression(Source source, String name, List<Expression> children, @N
         this(source, name, children, id, false);
     }
 
+    /**
+     * Assigns a new id if null is passed for {@code id}.
+     */
     public NamedExpression(Source source, String name, List<Expression> children, @Nullable NameId id, boolean synthetic) {
         super(source, children);
         this.name = name;
@@ -57,36 +60,53 @@ public boolean synthetic() {
     public abstract Attribute toAttribute();
 
     @Override
-    public int hashCode() {
-        return Objects.hash(super.hashCode(), name, synthetic);
+    public final int hashCode() {
+        return hashCode(false);
+    }
+
+    public final int hashCode(boolean ignoreIds) {
+        return innerHashCode(ignoreIds);
+    }
+
+    protected int innerHashCode(boolean ignoreIds) {
+        return ignoreIds ? Objects.hash(super.hashCode(), name, synthetic) : Objects.hash(super.hashCode(), id, name, synthetic);
     }
 
-    /**
-     * Polymorphic equality is a pain and are likely slower than a regular ones.
-     * This equals shortcuts `this == o` and type checks (important when we expect only a few non-equal objects).
-     * Here equals is final to ensure we are not duplicating those checks.
-     * For actual equality check override `innerEquals` instead.
-     */
     @Override
     public final boolean equals(Object o) {
+        return equals(o, false);
+    }
+
+    /**
+     * Polymorphic equality is a pain and can be slow.
+     * This shortcuts {@code this == o} and class checks (important when we expect only a few non-equal objects).
+     * <p>
+     * For the actual equality check override {@link #innerEquals(Object, boolean)} instead.
+     * <p>
+     * We also provide the option to ignore NameIds in the equality check, which helps e.g. when creating named expressions
+     * while avoiding duplicates, or when attaching failures to unresolved attributes (see Failure.equals).
+     * Some classes will always ignore ids, irrespective of the parameter passed here.
+     */
+    public final boolean equals(Object o, boolean ignoreIds) {
         if (this == o) {
             return true;
         }
         if (o == null || getClass() != o.getClass()) {
             return false;
         }
-        return innerEquals(o);
+        return innerEquals(o, ignoreIds);
     }
 
-    protected boolean innerEquals(Object o) {
+    /**
+     * The actual equality check, after shortcutting {@code this == o} and class checks.
+     */
+    protected boolean innerEquals(Object o, boolean ignoreIds) {
         var other = (NamedExpression) o;
-        return synthetic == other.synthetic
-            /*
-             * It is important that the line below be `name`
-             * and not `name()` because subclasses might override
-             * `name()` in ways that are not compatible with
-             * equality. Specifically the `Unresolved` subclasses.
-             */
+        return (ignoreIds || Objects.equals(id, other.id)) && synthetic == other.synthetic
+        // It is important that the line below be `name`
+        // and not `name()` because subclasses might override
+        // `name()` in ways that are not compatible with
+        // equality. Specifically the `Unresolved` subclasses.
             && Objects.equals(name, other.name)
             && Objects.equals(children(), other.children());
     }

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

@@ -12,6 +12,10 @@
 
 import java.util.Objects;
 
+/**
+ * A fully resolved attribute - we know its type. For example, if it references data directly from Lucene, this will be a
+ * {@link FieldAttribute}. If it references the results of another calculation it will be {@link ReferenceAttribute}s.
+ */
 public abstract class TypedAttribute extends Attribute {
 
     private final DataType dataType;
@@ -46,14 +50,13 @@ public DataType dataType() {
     }
 
     @Override
-    @SuppressWarnings("checkstyle:EqualsHashCode")// equals is implemented in parent. See innerEquals instead
-    public int hashCode() {
-        return Objects.hash(super.hashCode(), dataType);
+    protected int innerHashCode(boolean ignoreIds) {
+        return Objects.hash(super.innerHashCode(ignoreIds), dataType);
     }
 
     @Override
-    protected boolean innerEquals(Object o) {
+    protected boolean innerEquals(Object o, boolean ignoreIds) {
         var other = (TypedAttribute) o;
-        return super.innerEquals(other) && dataType == other.dataType;
+        return super.innerEquals(other, ignoreIds) && dataType == other.dataType;
     }
 }