From a932941be92d1e554901ebb31772b886b5d8afe7 Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Wed, 22 Oct 2025 13:16:28 +0200 Subject: [PATCH 1/2] ESQL: Make equals include ids for Alias, TypedAttribute (#132455) Fix https://github.com/elastic/elasticsearch/issues/131509 Fix https://github.com/elastic/elasticsearch/issues/132634 Make `Attribute#equals` and `Alias#equals` respect the `NameId`s, so that plan transformations that happen to replace an attribute/alias by one that only differs by id, we still actually update the plan rather than keeping the old plan object. This requires a bunch of boilerplate-y test updates. I'll point to the non-boilerplate changes in comments below. (cherry picked from commit f7d44220b5416d111cc097bbb58614285ea90734) # Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java --- docs/changelog/132455.yaml | 7 + .../xpack/esql/core/expression/Alias.java | 11 +- .../xpack/esql/core/expression/Attribute.java | 56 ++- .../esql/core/expression/EmptyAttribute.java | 18 +- .../esql/core/expression/Expression.java | 16 + .../esql/core/expression/FieldAttribute.java | 9 +- .../core/expression/MetadataAttribute.java | 9 +- .../xpack/esql/core/expression/NameId.java | 9 + .../esql/core/expression/NamedExpression.java | 54 ++- .../esql/core/expression/TypedAttribute.java | 13 +- .../core/expression/UnresolvedAttribute.java | 30 +- .../esql/core/expression/UnresolvedStar.java | 9 +- .../core/expression/AttributeMapTests.java | 24 +- .../expression/UnresolvedAttributeTests.java | 110 ------ .../xpack/esql/EsqlTestUtils.java | 100 +++++ .../src/main/resources/enrich.csv-spec | 18 + .../src/main/resources/lookup-join.csv-spec | 18 + .../src/main/resources/rename.csv-spec | 14 + .../xpack/esql/action/EsqlCapabilities.java | 9 +- .../xpack/esql/analysis/Analyzer.java | 22 +- .../xpack/esql/common/Failure.java | 15 + .../xpack/esql/common/Failures.java | 8 +- .../expression/UnresolvedNamePattern.java | 9 +- .../function/UnsupportedAttribute.java | 9 +- .../xpack/esql/io/stream/PlanStreamInput.java | 21 +- .../esql/plugin/ClusterComputeRequest.java | 9 +- .../xpack/esql/plugin/DataNodeRequest.java | 11 +- .../xpack/esql/SerializationTestUtils.java | 15 +- .../xpack/esql/analysis/AnalyzerTests.java | 49 +-- ...ractNamedExpressionSerializationTests.java | 38 ++ .../xpack/esql/expression/AliasTests.java | 34 +- .../expression/UnresolvedAttributeTests.java | 111 ++++++ .../UnresolvedNamePatternTests.java | 50 +++ .../esql/expression/UnresolvedStarTests.java | 47 +++ .../function/AbstractAttributeTestCase.java | 121 ------ .../function/FieldAttributeTests.java | 25 +- .../function/MetadataAttributeTests.java | 27 +- .../function/ReferenceAttributeTests.java | 25 +- .../function/UnresolvedFunctionTests.java | 2 +- .../function/UnsupportedAttributeTests.java | 24 +- .../aggregate/AvgSerializationTests.java | 8 +- .../aggregate/SumSerializationTests.java | 8 +- .../scalar/conditional/CaseExtraTests.java | 25 +- .../scalar/conditional/CaseTests.java | 5 +- .../esql/io/stream/PlanStreamOutputTests.java | 28 +- ...IgnoreNullMetricsPhysicalPlannerTests.java | 3 +- .../optimizer/LogicalPlanOptimizerTests.java | 21 +- .../esql/optimizer/OptimizerRulesTests.java | 4 +- .../parser/AbstractStatementParserTests.java | 4 +- .../xpack/esql/parser/ExpressionTests.java | 22 +- .../xpack/esql/parser/QualifierTests.java | 7 +- .../esql/parser/StatementParserTests.java | 357 +++++++++--------- .../plan/AbstractNodeSerializationTests.java | 13 +- .../ExchangeSinkExecSerializationTests.java | 10 +- .../esql/plugin/ClusterRequestTests.java | 3 +- .../DataNodeRequestSerializationTests.java | 4 +- .../esql/tree/EsqlNodeSubclassTests.java | 2 +- .../esql/type/AbstractEsFieldTypeTests.java | 22 +- .../xpack/esql/type/DateEsFieldTests.java | 2 +- .../xpack/esql/type/EsFieldTests.java | 2 +- .../esql/type/InvalidMappedFieldTests.java | 2 +- .../xpack/esql/type/KeywordEsFieldTests.java | 2 +- .../esql/type/MultiTypeEsFieldTests.java | 31 +- .../xpack/esql/type/TextEsFieldTests.java | 2 +- .../esql/type/UnsupportedEsFieldTests.java | 2 +- 65 files changed, 1126 insertions(+), 669 deletions(-) create mode 100644 docs/changelog/132455.yaml delete mode 100644 x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/UnresolvedAttributeTests.java create mode 100644 x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/AbstractNamedExpressionSerializationTests.java create mode 100644 x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/UnresolvedAttributeTests.java create mode 100644 x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/UnresolvedNamePatternTests.java create mode 100644 x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/UnresolvedStarTests.java delete mode 100644 x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAttributeTestCase.java diff --git a/docs/changelog/132455.yaml b/docs/changelog/132455.yaml new file mode 100644 index 0000000000000..e6860499ce858 --- /dev/null +++ b/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 diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Alias.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Alias.java index f0340f570c8b7..32259c50cb440 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Alias.java +++ b/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. - * + *

* 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. - * + *

+ * 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); diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Attribute.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Attribute.java index b7af6041ea56a..0ecac43c92359 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Attribute.java +++ b/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. - * + *

* 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. - * + *

* 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. + *

+ * 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. + *

+ * 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. + *

+ * 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 diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/EmptyAttribute.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/EmptyAttribute.java index 05fc864983d27..30c27d78cb88d 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/EmptyAttribute.java +++ b/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 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; + } } diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Expression.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Expression.java index 62c3adafb8fff..d283932d69f9b 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Expression.java +++ b/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. + *

+ * If two expressions are equal, they are also semantically equal, but the reverse is generally not true. + *

+ * 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. + *

+ * 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(); } diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/FieldAttribute.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/FieldAttribute.java index 837139eb6d552..1a292056119f4 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/FieldAttribute.java +++ b/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 diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/MetadataAttribute.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/MetadataAttribute.java index 79b2d9958cc09..2e0438797be60 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/MetadataAttribute.java +++ b/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; } } diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/NameId.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/NameId.java index 2ee600eb21e0b..f32775f94a298 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/NameId.java +++ b/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); diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/NamedExpression.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/NamedExpression.java index 1746b6308160c..80c7e0c6b8035 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/NamedExpression.java +++ b/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 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 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). + *

+ * For the actual equality check override {@link #innerEquals(Object, boolean)} instead. + *

+ * 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()); } diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/TypedAttribute.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/TypedAttribute.java index e7e40c7aacf1e..2705b8ccda2a8 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/TypedAttribute.java +++ b/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; } } diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/UnresolvedAttribute.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/UnresolvedAttribute.java index 5a31bd31d9189..ff19c06cbec9d 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/UnresolvedAttribute.java +++ b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/UnresolvedAttribute.java @@ -20,18 +20,19 @@ import java.util.Objects; /** - * An unresolved attribute. We build these while walking the syntax - * tree and then resolve them into other {@link Attribute} subclasses during - * analysis. + * An unresolved attribute. We build these while walking the syntax tree and then resolve them into other {@link Attribute} subclasses + * during analysis. {@link Alias}es whose types are not yet resolved also return {@link UnresolvedAttribute}s when calling + * {@link Alias#toAttribute()}. *

- * For example, if they reference the data directly from lucene they'll be - * {@link FieldAttribute}s. If they reference the results of another calculation - * they will be {@link ReferenceAttribute}s. - *

+ * Note that the {@link #id()} is respected in {@link #equals(Object)}. Two unresolved attributes with the same name but different + * {@link NameId} can occur e.g. when resolving {@code EVAL x = 2*x, y = 3*x}. In the first expression, {@code x} is referring to an + * upstream attribute, while in the second expression it is referring to the alias defined in the first expression. This allows us to + * distinguish the attributes generated by the {@code EVAL} from attributes referenced by it. */ public class UnresolvedAttribute extends Attribute implements Unresolvable { private final boolean customMessage; private final String unresolvedMsg; + // TODO: unused and always null, remove this. private final Object resolutionMetadata; // TODO: Check usage of constructors without qualifiers, that's likely where qualifiers need to be plugged into resolution logic. @@ -116,6 +117,12 @@ protected Attribute clone( return this; } + // Cannot just use the super method because that requires a data type. + @Override + public UnresolvedAttribute withId(NameId id) { + return new UnresolvedAttribute(source(), qualifier(), name(), id, unresolvedMessage(), resolutionMetadata()); + } + public UnresolvedAttribute withUnresolvedMessage(String unresolvedMessage) { return new UnresolvedAttribute(source(), qualifier(), name(), id(), unresolvedMessage, resolutionMetadata()); } @@ -173,15 +180,14 @@ public static String errorMessage(String name, List potentialMatches) { } @Override - @SuppressWarnings("checkstyle:EqualsHashCode")// equals is implemented in parent. See innerEquals instead - public int hashCode() { - return Objects.hash(super.hashCode(), resolutionMetadata, unresolvedMsg); + protected int innerHashCode(boolean ignoreIds) { + return Objects.hash(super.innerHashCode(ignoreIds), resolutionMetadata, unresolvedMsg); } @Override - protected boolean innerEquals(Object o) { + protected boolean innerEquals(Object o, boolean ignoreIds) { var other = (UnresolvedAttribute) o; - return super.innerEquals(other) + return super.innerEquals(o, ignoreIds) && Objects.equals(resolutionMetadata, other.resolutionMetadata) && Objects.equals(unresolvedMsg, other.unresolvedMsg); } diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/UnresolvedStar.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/UnresolvedStar.java index 2508702a87156..273f68fc32e85 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/UnresolvedStar.java +++ b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/UnresolvedStar.java @@ -19,7 +19,7 @@ public class UnresolvedStar extends UnresolvedNamedExpression { - // typically used for nested fields or inner/dotted fields + // TODO: Currently unused, can be removed. (Qualifiers will likely remain just strings.) private final UnresolvedAttribute qualifier; public UnresolvedStar(Source source, UnresolvedAttribute qualifier) { @@ -57,15 +57,14 @@ public UnresolvedAttribute qualifier() { } @Override - @SuppressWarnings("checkstyle:EqualsHashCode")// equals is implemented in parent. See innerEquals instead - public int hashCode() { + protected int innerHashCode(boolean ignoreIds) { return Objects.hash(qualifier); } @Override - protected boolean innerEquals(Object o) { + protected boolean innerEquals(Object o, boolean ignoreIds) { var other = (UnresolvedStar) o; - return super.innerEquals(other) && Objects.equals(qualifier, other.qualifier); + return super.innerEquals(other, true) && Objects.equals(qualifier, other.qualifier); } private String message() { diff --git a/x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/AttributeMapTests.java b/x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/AttributeMapTests.java index b7585a54495cb..205aa724ff635 100644 --- a/x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/AttributeMapTests.java +++ b/x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/AttributeMapTests.java @@ -34,11 +34,15 @@ static Attribute a(String name) { return new UnresolvedAttribute(Source.EMPTY, name); } + private static Attribute ONE = a("one"); + private static Attribute TWO = a("two"); + private static Attribute THREE = a("three"); + private static AttributeMap threeMap() { AttributeMap.Builder builder = AttributeMap.builder(); - builder.put(a("one"), "one"); - builder.put(a("two"), "two"); - builder.put(a("three"), "three"); + builder.put(ONE, "one"); + builder.put(TWO, "two"); + builder.put(THREE, "three"); return builder.build(); } @@ -46,12 +50,12 @@ private static AttributeMap threeMap() { public void testAttributeMapWithSameAliasesCanResolveAttributes() { Alias param1 = createIntParameterAlias(1, 100); Alias param2 = createIntParameterAlias(2, 100); - assertTrue(param1.equals(param2)); - assertTrue(param1.semanticEquals(param2)); + assertFalse(param1.equals(param2)); + assertFalse(param1.semanticEquals(param2)); // equality on literals assertTrue(param1.child().equals(param2.child())); assertTrue(param1.child().semanticEquals(param2.child())); - assertTrue(param1.toAttribute().equals(param2.toAttribute())); + assertFalse(param1.toAttribute().equals(param2.toAttribute())); assertFalse(param1.toAttribute().semanticEquals(param2.toAttribute())); AttributeMap.Builder mapBuilder = AttributeMap.builder(); @@ -205,18 +209,14 @@ public void testSubsetOf() { } public void testKeySet() { - Attribute one = a("one"); - Attribute two = a("two"); - Attribute three = a("three"); - Set keySet = threeMap().keySet(); - assertThat(keySet, contains(one, two, three)); + assertThat(keySet, contains(ONE, TWO, THREE)); // toObject Object[] array = keySet.toArray(); assertThat(array, arrayWithSize(3)); - assertThat(array, arrayContaining(one, two, three)); + assertThat(array, arrayContaining(ONE, TWO, THREE)); } public void testValues() { diff --git a/x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/UnresolvedAttributeTests.java b/x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/UnresolvedAttributeTests.java deleted file mode 100644 index 6ac8377a2d8b9..0000000000000 --- a/x-pack/plugin/esql-core/src/test/java/org/elasticsearch/xpack/esql/core/expression/UnresolvedAttributeTests.java +++ /dev/null @@ -1,110 +0,0 @@ -/* - * 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.xpack.esql.core.tree.AbstractNodeTestCase; -import org.elasticsearch.xpack.esql.core.tree.Source; -import org.elasticsearch.xpack.esql.core.tree.SourceTests; - -import java.util.Arrays; -import java.util.Objects; -import java.util.function.Supplier; - -public class UnresolvedAttributeTests extends AbstractNodeTestCase { - public static UnresolvedAttribute randomUnresolvedAttribute() { - Source source = SourceTests.randomSource(); - String name = randomAlphaOfLength(5); - NameId id = randomBoolean() ? null : new NameId(); - String unresolvedMessage = randomUnresolvedMessage(); - Object resolutionMetadata = new Object(); - return new UnresolvedAttribute(source, name, id, unresolvedMessage, resolutionMetadata); - } - - /** - * A random qualifier. It is important that this be distinct - * from the name and the unresolvedMessage for testing transform. - */ - private static String randomQualifier() { - return randomBoolean() ? null : randomAlphaOfLength(6); - } - - /** - * A random qualifier. It is important that this be distinct - * from the name and the qualifier for testing transform. - */ - private static String randomUnresolvedMessage() { - return randomAlphaOfLength(7); - } - - @Override - protected UnresolvedAttribute randomInstance() { - return randomUnresolvedAttribute(); - } - - @Override - protected UnresolvedAttribute mutate(UnresolvedAttribute a) { - Supplier option = randomFrom( - Arrays.asList( - () -> new UnresolvedAttribute( - a.source(), - randomValueOtherThan(a.name(), () -> randomAlphaOfLength(5)), - a.id(), - a.unresolvedMessage(), - a.resolutionMetadata() - ), - () -> new UnresolvedAttribute( - a.source(), - a.name(), - a.id(), - randomValueOtherThan(a.unresolvedMessage(), () -> randomUnresolvedMessage()), - a.resolutionMetadata() - ), - () -> new UnresolvedAttribute(a.source(), a.name(), a.id(), a.unresolvedMessage(), new Object()) - ) - ); - return option.get(); - } - - @Override - protected UnresolvedAttribute copy(UnresolvedAttribute a) { - return new UnresolvedAttribute(a.source(), a.name(), a.id(), a.unresolvedMessage(), a.resolutionMetadata()); - } - - @Override - public void testTransform() { - UnresolvedAttribute a = randomUnresolvedAttribute(); - - String newName = randomValueOtherThan(a.name(), () -> randomAlphaOfLength(5)); - assertEquals( - new UnresolvedAttribute(a.source(), newName, a.id(), a.unresolvedMessage(), a.resolutionMetadata()), - a.transformPropertiesOnly(Object.class, v -> Objects.equals(v, a.name()) ? newName : v) - ); - - NameId newId = new NameId(); - assertEquals( - new UnresolvedAttribute(a.source(), a.name(), newId, a.unresolvedMessage(), a.resolutionMetadata()), - a.transformPropertiesOnly(Object.class, v -> Objects.equals(v, a.id()) ? newId : v) - ); - - String newMessage = randomValueOtherThan(a.unresolvedMessage(), UnresolvedAttributeTests::randomUnresolvedMessage); - assertEquals( - new UnresolvedAttribute(a.source(), a.name(), a.id(), newMessage, a.resolutionMetadata()), - a.transformPropertiesOnly(Object.class, v -> Objects.equals(v, a.unresolvedMessage()) ? newMessage : v) - ); - - Object newMeta = new Object(); - assertEquals( - new UnresolvedAttribute(a.source(), a.name(), a.id(), a.unresolvedMessage(), newMeta), - a.transformPropertiesOnly(Object.class, v -> Objects.equals(v, a.resolutionMetadata()) ? newMeta : v) - ); - } - - @Override - public void testReplaceChildren() { - // UnresolvedAttribute doesn't have any children - } -} diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java index 8e29b0f27286d..0fb0db111c4a8 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java @@ -64,12 +64,15 @@ import org.elasticsearch.xpack.esql.analysis.AnalyzerSettings; import org.elasticsearch.xpack.esql.analysis.EnrichResolution; import org.elasticsearch.xpack.esql.analysis.Verifier; +import org.elasticsearch.xpack.esql.core.expression.Alias; import org.elasticsearch.xpack.esql.core.expression.Attribute; import org.elasticsearch.xpack.esql.core.expression.Expression; import org.elasticsearch.xpack.esql.core.expression.FieldAttribute; import org.elasticsearch.xpack.esql.core.expression.FieldAttribute.FieldName; import org.elasticsearch.xpack.esql.core.expression.FoldContext; import org.elasticsearch.xpack.esql.core.expression.Literal; +import org.elasticsearch.xpack.esql.core.expression.NameId; +import org.elasticsearch.xpack.esql.core.expression.NamedExpression; import org.elasticsearch.xpack.esql.core.expression.ReferenceAttribute; import org.elasticsearch.xpack.esql.core.expression.predicate.regex.RLikePattern; import org.elasticsearch.xpack.esql.core.expression.predicate.regex.WildcardPattern; @@ -99,11 +102,14 @@ import org.elasticsearch.xpack.esql.parser.QueryParam; import org.elasticsearch.xpack.esql.plan.logical.Enrich; import org.elasticsearch.xpack.esql.plan.logical.EsRelation; +import org.elasticsearch.xpack.esql.plan.logical.Explain; import org.elasticsearch.xpack.esql.plan.logical.Limit; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.esql.plan.logical.local.EmptyLocalSupplier; import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation; import org.elasticsearch.xpack.esql.plan.logical.local.LocalSupplier; +import org.elasticsearch.xpack.esql.plan.physical.FragmentExec; +import org.elasticsearch.xpack.esql.plan.physical.PhysicalPlan; import org.elasticsearch.xpack.esql.planner.PlannerSettings; import org.elasticsearch.xpack.esql.planner.PlannerUtils; import org.elasticsearch.xpack.esql.plugin.QueryPragmas; @@ -112,6 +118,10 @@ import org.elasticsearch.xpack.esql.stats.SearchStats; import org.elasticsearch.xpack.esql.telemetry.Metrics; import org.elasticsearch.xpack.versionfield.Version; +import org.hamcrest.Matcher; +import org.hamcrest.collection.IsIterableContainingInAnyOrder; +import org.hamcrest.collection.IsIterableContainingInOrder; +import org.hamcrest.core.IsEqual; import org.junit.Assert; import java.io.BufferedReader; @@ -1078,4 +1088,94 @@ public static Exception unwrapIfWrappedInRemoteException(Exception e) { return e; } } + + public static void assertEqualsIgnoringIds(Object expected, Object actual) { + assertEqualsIgnoringIds("", expected, actual); + } + + public static void assertEqualsIgnoringIds(String reason, Object expected, Object actual) { + assertThat(reason, actual, equalToIgnoringIds(expected)); + } + + /** + * Returns a matcher that matches if the examined object is logically equal to the specified + * operand, ignoring the {@link NameId}s of any {@link NamedExpression}s (e.g. {@link Alias} and {@link Attribute}). + */ + public static org.hamcrest.Matcher equalToIgnoringIds(T operand) { + return new IsEqualIgnoringIds(operand); + } + + @SafeVarargs + public static org.hamcrest.Matcher> containsIgnoringIds(T... items) { + List> matchers = new ArrayList<>(); + for (T item : items) { + matchers.add(equalToIgnoringIds(item)); + } + + return new IsIterableContainingInOrder<>(matchers); + } + + @SafeVarargs + public static org.hamcrest.Matcher> containsInAnyOrderIgnoringIds(T... items) { + List> matchers = new ArrayList<>(); + for (T item : items) { + matchers.add(equalToIgnoringIds(item)); + } + + return new IsIterableContainingInAnyOrder<>(matchers); + } + + private static class IsEqualIgnoringIds extends IsEqual { + @SuppressWarnings("unchecked") + IsEqualIgnoringIds(T operand) { + super((T) ignoreIds(operand)); + } + + @Override + public boolean matches(Object actualValue) { + return super.matches(ignoreIds(actualValue)); + } + } + + public static Object ignoreIds(Object node) { + return switch (node) { + case Expression expression -> ignoreIdsInExpression(expression); + case LogicalPlan plan -> ignoreIdsInLogicalPlan(plan); + case PhysicalPlan pplan -> ignoreIdsInPhysicalPlan(pplan); + case List list -> list.stream().map(EsqlTestUtils::ignoreIds).toList(); + case null, default -> node; + }; + } + + private static final NameId DUMMY_ID = new NameId(); + + private static Expression ignoreIdsInExpression(Expression expression) { + return expression.transformDown( + NamedExpression.class, + ne -> ne instanceof Alias alias ? alias.withId(DUMMY_ID) : ne instanceof Attribute attr ? attr.withId(DUMMY_ID) : ne + ); + } + + private static LogicalPlan ignoreIdsInLogicalPlan(LogicalPlan plan) { + if (plan instanceof Explain explain) { + return new Explain(explain.source(), ignoreIdsInLogicalPlan(explain.query())); + } + + return plan.transformExpressionsDown( + NamedExpression.class, + ne -> ne instanceof Alias alias ? alias.withId(DUMMY_ID) : ne instanceof Attribute attr ? attr.withId(DUMMY_ID) : ne + ); + } + + private static PhysicalPlan ignoreIdsInPhysicalPlan(PhysicalPlan plan) { + PhysicalPlan ignoredInPhysicalNodes = plan.transformExpressionsDown( + NamedExpression.class, + ne -> ne instanceof Alias alias ? alias.withId(DUMMY_ID) : ne instanceof Attribute attr ? attr.withId(DUMMY_ID) : ne + ); + return ignoredInPhysicalNodes.transformDown(FragmentExec.class, fragmentExec -> { + LogicalPlan fragment = fragmentExec.fragment(); + LogicalPlan ignoredInFragment = ignoreIdsInLogicalPlan(fragment); + return fragmentExec.withFragment(ignoredInFragment); + }); + } } diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/enrich.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/enrich.csv-spec index 1f8919fc7cd32..f60b876a1c7ee 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/enrich.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/enrich.csv-spec @@ -763,3 +763,21 @@ FROM sample_data message:keyword | language_code:keyword | first_language_name:keyword | language_name:keyword Connected to 10.1.0.1 | 1 | English | English ; + +enrichOnSameFieldTwiceValues +required_capability: enrich_load +required_capability: attribute_equals_respects_name_id + +ROW a = 1, b = 2 +| EVAL language_code = a +| ENRICH languages_policy ON language_code +| RENAME language_name AS language_name_a +| EVAL language_code = b +| ENRICH languages_policy ON language_code +| RENAME language_name AS language_name_b +| STATS values(language_name_a), values(language_name_b) +; + +values(language_name_a):keyword | values(language_name_b):keyword +English | French +; diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec index f5d5c9e45c706..2acf6344d7be1 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec @@ -1892,6 +1892,24 @@ type:keyword | language_code:integer | language_name:keyword Production | 3 | Spanish ; +lookupJoinOnSameFieldTwiceValues +required_capability: join_lookup_v12 +required_capability: attribute_equals_respects_name_id + +ROW a = 1, b = 2 +| EVAL language_code = a +| LOOKUP JOIN languages_lookup ON language_code +| RENAME language_name AS language_name_a +| EVAL language_code = b +| LOOKUP JOIN languages_lookup ON language_code +| RENAME language_name AS language_name_b +| STATS values(language_name_a), values(language_name_b) +; + +values(language_name_a):keyword | values(language_name_b):keyword +English | French +; + ############################################### # LOOKUP JOIN on mixed numerical fields ############################################### diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/rename.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/rename.csv-spec index f485e39233982..936723536099a 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/rename.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/rename.csv-spec @@ -343,3 +343,17 @@ ROW a="keyword", b=5, c=null a:integer 5 ; + +circularRename +required_capability: attribute_equals_respects_name_id + +FROM employees +| EVAL y = emp_no - 10000 +| RENAME y as z, z as y +| WHERE y*y == 1 +| KEEP emp_no, y +; + +emp_no:integer | y:integer +10001 | 1 +; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java index 254bed423b094..996abc3c3dfd7 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java @@ -1618,8 +1618,15 @@ public enum Cap { /** * Fix double release in inline stats when LocalRelation is reused */ - INLINE_STATS_DOUBLE_RELEASE_FIX(INLINESTATS_V11.enabled) + INLINE_STATS_DOUBLE_RELEASE_FIX(INLINESTATS_V11.enabled), + /** + * Fix attribute equality to respect the name id of the attribute. + */ + ATTRIBUTE_EQUALS_RESPECTS_NAME_ID, + + // Last capability should still have a comma for fewer merge conflicts when adding new ones :) + // This comment prevents the semicolon from being on the previous capability when Spotless formats the file. ; private final boolean enabled; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java index bbd0865f9a98c..bfb9f5a567e7f 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java @@ -1855,18 +1855,18 @@ record TypeResolutionKey(String fieldName, DataType fieldType) {} @Override public LogicalPlan apply(LogicalPlan plan) { - List unionFieldAttributes = new ArrayList<>(); + List unionFieldAttributes = new ArrayList<>(); return plan.transformUp(LogicalPlan.class, p -> p.childrenResolved() == false ? p : doRule(p, unionFieldAttributes)); } - private LogicalPlan doRule(LogicalPlan plan, List unionFieldAttributes) { + private LogicalPlan doRule(LogicalPlan plan, List unionFieldAttributes) { Holder alreadyAddedUnionFieldAttributes = new Holder<>(unionFieldAttributes.size()); // Collect field attributes from previous runs if (plan instanceof EsRelation rel) { unionFieldAttributes.clear(); for (Attribute attr : rel.output()) { if (attr instanceof FieldAttribute fa && fa.field() instanceof MultiTypeEsField && fa.synthetic()) { - unionFieldAttributes.add(fa); + unionFieldAttributes.add(fa.ignoreId()); } } } @@ -1885,7 +1885,7 @@ private LogicalPlan doRule(LogicalPlan plan, List unionFieldAttr return plan; } - return addGeneratedFieldsToEsRelations(plan, unionFieldAttributes); + return addGeneratedFieldsToEsRelations(plan, unionFieldAttributes.stream().map(attr -> (FieldAttribute) attr.inner()).toList()); } /** @@ -1915,7 +1915,7 @@ private static LogicalPlan addGeneratedFieldsToEsRelations(LogicalPlan plan, Lis }); } - private Expression resolveConvertFunction(ConvertFunction convert, List unionFieldAttributes) { + private Expression resolveConvertFunction(ConvertFunction convert, List unionFieldAttributes) { Expression convertExpression = (Expression) convert; if (convert.field() instanceof FieldAttribute fa && fa.field() instanceof InvalidMappedField imf) { HashMap typeResolutions = new HashMap<>(); @@ -1986,7 +1986,7 @@ private Expression resolveConvertFunction(ConvertFunction convert, List unionFieldAttributes + List unionFieldAttributes ) { // Generate new ID for the field and suffix it with the data type to maintain unique attribute names. // NOTE: The name has to start with $$ to not break bwc with 8.15 - in that version, this is how we had to mark this as @@ -2000,13 +2000,15 @@ private Expression createIfDoesNotAlreadyExist( resolvedField, true ); - int existingIndex = unionFieldAttributes.indexOf(unionFieldAttribute); + var nonSemanticUnionFieldAttribute = unionFieldAttribute.ignoreId(); + + int existingIndex = unionFieldAttributes.indexOf(nonSemanticUnionFieldAttribute); if (existingIndex >= 0) { // Do not generate multiple name/type combinations with different IDs - return unionFieldAttributes.get(existingIndex); + return unionFieldAttributes.get(existingIndex).inner(); } else { - unionFieldAttributes.add(unionFieldAttribute); - return unionFieldAttribute; + unionFieldAttributes.add(nonSemanticUnionFieldAttribute); + return nonSemanticUnionFieldAttribute.inner(); } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/common/Failure.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/common/Failure.java index e5d0fb7ba0b3d..1cc56343fa54f 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/common/Failure.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/common/Failure.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.esql.common; +import org.elasticsearch.xpack.esql.core.expression.UnresolvedAttribute; import org.elasticsearch.xpack.esql.core.tree.Location; import org.elasticsearch.xpack.esql.core.tree.Node; import org.elasticsearch.xpack.esql.core.util.StringUtils; @@ -37,9 +38,15 @@ public String message() { @Override public int hashCode() { + if (node instanceof UnresolvedAttribute ua) { + return ua.hashCode(true); + } return Objects.hash(node); } + /** + * Equality is based on the contained node, the failure is "attached" to it. + */ @Override public boolean equals(Object obj) { if (this == obj) { @@ -51,6 +58,14 @@ public boolean equals(Object obj) { } Failure other = (Failure) obj; + + // When deduplicating failures, it's important to ignore the NameIds of UnresolvedAttributes. + // Otherwise, two failures will be emitted to the user for e.g. + // `FROM test | STATS max(unknown) by unknown` because the two `unknown` attributes will have differentNameIds - even though they + // clearly refer to the same problem. + if (node instanceof UnresolvedAttribute ua) { + return ua.equals(other.node, true); + } return Objects.equals(node, other.node); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/common/Failures.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/common/Failures.java index fd25cb427d95b..1720cd3266796 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/common/Failures.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/common/Failures.java @@ -7,16 +7,16 @@ package org.elasticsearch.xpack.esql.common; -import java.util.Collection; import java.util.LinkedHashSet; import java.util.Objects; +import java.util.Set; /** - * Glorified list for managing {@link Failure}s. + * Glorified set for managing {@link Failure}s. */ public class Failures { - private final Collection failures; + private final Set failures; public Failures() { this.failures = new LinkedHashSet<>(); @@ -33,7 +33,7 @@ public boolean hasFailures() { return failures.size() > 0; } - public Collection failures() { + public Set failures() { return failures; } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/UnresolvedNamePattern.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/UnresolvedNamePattern.java index 097d42e947417..986e715cce681 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/UnresolvedNamePattern.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/UnresolvedNamePattern.java @@ -98,15 +98,14 @@ public Nullability nullable() { } @Override - @SuppressWarnings("checkstyle:EqualsHashCode")// equals is implemented in parent. See innerEquals instead - public int hashCode() { - return Objects.hash(super.hashCode(), pattern); + protected int innerHashCode(boolean ignoreIds) { + return Objects.hash(super.innerHashCode(true), pattern); } @Override - protected boolean innerEquals(Object o) { + protected boolean innerEquals(Object o, boolean ignoreIds) { var other = (UnresolvedNamePattern) o; - return super.innerEquals(other) && Objects.equals(pattern, other.pattern); + return super.innerEquals(other, true) && Objects.equals(pattern, other.pattern); } @Override diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/UnsupportedAttribute.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/UnsupportedAttribute.java index e675fb48ff80e..c003b5c7dec6d 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/UnsupportedAttribute.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/UnsupportedAttribute.java @@ -178,15 +178,14 @@ public boolean hasCustomMessage() { } @Override - @SuppressWarnings("checkstyle:EqualsHashCode")// equals is implemented in parent. See innerEquals instead - public int hashCode() { - return Objects.hash(super.hashCode(), hasCustomMessage, message); + protected int innerHashCode(boolean ignoreIds) { + return Objects.hash(super.innerHashCode(ignoreIds), hasCustomMessage, message); } @Override - protected boolean innerEquals(Object o) { + protected boolean innerEquals(Object o, boolean ignoreIds) { var other = (UnsupportedAttribute) o; - return super.innerEquals(other) && hasCustomMessage == other.hasCustomMessage && Objects.equals(message, other.message); + return super.innerEquals(other, ignoreIds) && hasCustomMessage == other.hasCustomMessage && Objects.equals(message, other.message); } /** diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/io/stream/PlanStreamInput.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/io/stream/PlanStreamInput.java index ca24f1ad322a8..6544a307de470 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/io/stream/PlanStreamInput.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/io/stream/PlanStreamInput.java @@ -51,13 +51,17 @@ public final class PlanStreamInput extends NamedWriteableAwareStreamInput * and increment an id from the global counter, thus avoiding potential conflicts between the * id in the stream and id's during local re-planning on the data node. */ - static final class NameIdMapper implements LongFunction { + public static class NameIdMapper implements LongFunction { final Map seen = new HashMap<>(); @Override public NameId apply(long streamNameId) { return seen.computeIfAbsent(streamNameId, k -> new NameId()); } + + protected Map seen() { + return seen; + } } private final Map cachedBlocks = new HashMap<>(); @@ -74,9 +78,22 @@ public NameId apply(long streamNameId) { private final Configuration configuration; public PlanStreamInput(StreamInput streamInput, NamedWriteableRegistry namedWriteableRegistry, Configuration configuration) { + this(streamInput, namedWriteableRegistry, configuration, null); + } + + /** + * @param idMapper should always be null in production! Custom mappers are only used in tests to force ID values to be the same after + * serialization and deserialization, which is not the case when they are generated as usual. + */ + public PlanStreamInput( + StreamInput streamInput, + NamedWriteableRegistry namedWriteableRegistry, + Configuration configuration, + NameIdMapper idMapper + ) { super(streamInput, namedWriteableRegistry); this.configuration = configuration; - this.nameIdFunction = new NameIdMapper(); + this.nameIdFunction = idMapper == null ? new NameIdMapper() : idMapper; } public Configuration configuration() throws IOException { diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/ClusterComputeRequest.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/ClusterComputeRequest.java index d83ca4beef52c..28cef1bcab8ce 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/ClusterComputeRequest.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/ClusterComputeRequest.java @@ -60,6 +60,13 @@ final class ClusterComputeRequest extends AbstractTransportRequest implements In } ClusterComputeRequest(StreamInput in) throws IOException { + this(in, null); + } + + /** + * @param idMapper must always be null in production. Only used in tests to remap NameIds when deserializing. + */ + ClusterComputeRequest(StreamInput in, PlanStreamInput.NameIdMapper idMapper) throws IOException { super(in); this.clusterAlias = in.readString(); this.sessionId = in.readString(); @@ -67,7 +74,7 @@ final class ClusterComputeRequest extends AbstractTransportRequest implements In // TODO make EsqlConfiguration Releasable new BlockStreamInput(in, new BlockFactory(new NoopCircuitBreaker(CircuitBreaker.REQUEST), BigArrays.NON_RECYCLING_INSTANCE)) ); - this.plan = RemoteClusterPlan.from(new PlanStreamInput(in, in.namedWriteableRegistry(), configuration)); + this.plan = RemoteClusterPlan.from(new PlanStreamInput(in, in.namedWriteableRegistry(), configuration, idMapper)); this.indices = plan.originalIndices().indices(); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequest.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequest.java index 7d06a49d53c12..e8b21925b4a0f 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequest.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequest.java @@ -78,6 +78,14 @@ final class DataNodeRequest extends AbstractTransportRequest implements IndicesR } DataNodeRequest(StreamInput in) throws IOException { + this(in, null); + } + + /** + * @param idMapper should always be null in production! Custom mappers are only used in tests to force ID values to be the same after + * serialization and deserialization, which is not the case when they are generated as usual. + */ + DataNodeRequest(StreamInput in, PlanStreamInput.NameIdMapper idMapper) throws IOException { super(in); this.sessionId = in.readString(); this.configuration = new Configuration( @@ -87,7 +95,8 @@ final class DataNodeRequest extends AbstractTransportRequest implements IndicesR this.clusterAlias = in.readString(); this.shardIds = in.readCollectionAsList(ShardId::new); this.aliasFilters = in.readMap(Index::new, AliasFilter::readFrom); - this.plan = new PlanStreamInput(in, in.namedWriteableRegistry(), configuration).readNamedWriteable(PhysicalPlan.class); + PlanStreamInput pin = new PlanStreamInput(in, in.namedWriteableRegistry(), configuration, idMapper); + this.plan = pin.readNamedWriteable(PhysicalPlan.class); this.indices = in.readStringArray(); this.indicesOptions = IndicesOptions.readIndicesOptions(in); if (in.getTransportVersion().supports(TransportVersions.V_8_18_0)) { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/SerializationTestUtils.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/SerializationTestUtils.java index e55a1b039258e..9521a74c1db2b 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/SerializationTestUtils.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/SerializationTestUtils.java @@ -27,6 +27,7 @@ import org.elasticsearch.search.vectors.KnnVectorQueryBuilder; import org.elasticsearch.test.EqualsHashCodeTestUtils; import org.elasticsearch.xpack.esql.core.expression.Expression; +import org.elasticsearch.xpack.esql.core.expression.NameId; import org.elasticsearch.xpack.esql.expression.ExpressionWritables; import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput; import org.elasticsearch.xpack.esql.io.stream.PlanStreamOutput; @@ -83,11 +84,12 @@ public static T serializeDeserialize(T orig, Serializer serializer, Deser try (BytesStreamOutput out = new BytesStreamOutput()) { PlanStreamOutput planStreamOutput = new PlanStreamOutput(out, config); serializer.write(planStreamOutput, orig); + StreamInput in = new NamedWriteableAwareStreamInput( ByteBufferStreamInput.wrap(BytesReference.toBytes(out.bytes())), writableRegistry() ); - PlanStreamInput planStreamInput = new PlanStreamInput(in, in.namedWriteableRegistry(), config); + PlanStreamInput planStreamInput = new PlanStreamInput(in, in.namedWriteableRegistry(), config, new TestNameIdMapper()); return deserializer.read(planStreamInput); } catch (IOException e) { throw new UncheckedIOException(e); @@ -125,4 +127,15 @@ public static NamedWriteableRegistry writableRegistry() { ); return new NamedWriteableRegistry(entries); } + + /** + * Maps NameIds seen in a plan to themselves rather than creating new, unique ones. + * This makes equality checks easier when comparing a plan to itself after serialization and deserialization. + */ + public static class TestNameIdMapper extends PlanStreamInput.NameIdMapper { + @Override + public NameId apply(long streamNameId) { + return new NameId(streamNameId); + } + } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java index ac9edac5704fe..a9be6f2832bf5 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java @@ -116,6 +116,7 @@ import static org.elasticsearch.xpack.esql.EsqlTestUtils.as; import static org.elasticsearch.xpack.esql.EsqlTestUtils.configuration; import static org.elasticsearch.xpack.esql.EsqlTestUtils.emptyInferenceResolution; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.equalToIgnoringIds; import static org.elasticsearch.xpack.esql.EsqlTestUtils.getAttributeByName; import static org.elasticsearch.xpack.esql.EsqlTestUtils.paramAsConstant; import static org.elasticsearch.xpack.esql.EsqlTestUtils.paramAsIdentifier; @@ -215,7 +216,10 @@ public void testAttributeResolution() { var limit = as(plan, Limit.class); var eval = as(limit.child(), Eval.class); assertEquals(1, eval.fields().size()); - assertEquals(new Alias(EMPTY, "e", new FieldAttribute(EMPTY, "emp_no", idx.mapping().get("emp_no"))), eval.fields().get(0)); + assertThat( + eval.fields().get(0), + equalToIgnoringIds(new Alias(EMPTY, "e", new FieldAttribute(EMPTY, "emp_no", idx.mapping().get("emp_no")))) + ); assertEquals(2, eval.output().size()); Attribute empNo = eval.output().get(0); @@ -272,7 +276,10 @@ public void testRowAttributeResolution() { var limit = as(plan, Limit.class); var eval = as(limit.child(), Eval.class); assertEquals(1, eval.fields().size()); - assertEquals(new Alias(EMPTY, "e", new ReferenceAttribute(EMPTY, "emp_no", DataType.INTEGER)), eval.fields().get(0)); + assertThat( + eval.fields().get(0), + equalToIgnoringIds(new Alias(EMPTY, "e", new ReferenceAttribute(EMPTY, "emp_no", DataType.INTEGER))) + ); assertEquals(2, eval.output().size()); Attribute empNo = eval.output().get(0); @@ -3142,8 +3149,8 @@ public void testResolveInsist_fieldDoesNotExist_createsUnmappedField() { var insist = as(limit.child(), Insist.class); assertThat(insist.output(), hasSize(analyze("FROM test").output().size() + 1)); var expectedAttribute = new FieldAttribute(Source.EMPTY, "foo", new PotentiallyUnmappedKeywordEsField("foo")); - assertThat(insist.insistedAttributes(), is(List.of(expectedAttribute))); - assertThat(insist.output().getLast(), is(expectedAttribute)); + assertThat(insist.insistedAttributes(), equalToIgnoringIds(List.of(expectedAttribute))); + assertThat(insist.output().getLast(), equalToIgnoringIds(expectedAttribute)); } public void testResolveInsist_multiIndexFieldPartiallyMappedWithSingleKeywordType_createsUnmappedField() { @@ -3384,7 +3391,7 @@ public void testBasicFork() { List projectColumns = project.expressions().stream().map(exp -> as(exp, Attribute.class).name()).toList(); assertThat(projectColumns, equalTo(expectedOutput)); Eval eval = as(project.child(), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", string("fork1")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", string("fork1")))); Filter filter = as(eval.child(), Filter.class); assertThat(as(filter.condition(), GreaterThan.class).right(), equalTo(literal(1))); @@ -3401,7 +3408,7 @@ public void testBasicFork() { projectColumns = project.expressions().stream().map(exp -> as(exp, Attribute.class).name()).toList(); assertThat(projectColumns, equalTo(expectedOutput)); eval = as(project.child(), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", string("fork2")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", string("fork2")))); filter = as(eval.child(), Filter.class); assertThat(as(filter.condition(), GreaterThan.class).right(), equalTo(literal(2))); @@ -3418,7 +3425,7 @@ public void testBasicFork() { projectColumns = project.expressions().stream().map(exp -> as(exp, Attribute.class).name()).toList(); assertThat(projectColumns, equalTo(expectedOutput)); eval = as(project.child(), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", string("fork3")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", string("fork3")))); limit = as(eval.child(), Limit.class); assertThat(as(limit.limit(), Literal.class).value(), equalTo(7)); var orderBy = as(limit.child(), OrderBy.class); @@ -3437,7 +3444,7 @@ public void testBasicFork() { projectColumns = project.expressions().stream().map(exp -> as(exp, Attribute.class).name()).toList(); assertThat(projectColumns, equalTo(expectedOutput)); eval = as(project.child(), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", string("fork4")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", string("fork4")))); orderBy = as(eval.child(), OrderBy.class); filter = as(orderBy.child(), Filter.class); assertThat(as(filter.condition(), Equals.class).right(), equalTo(string("Chris"))); @@ -3452,7 +3459,7 @@ public void testBasicFork() { projectColumns = project.expressions().stream().map(exp -> as(exp, Attribute.class).name()).toList(); assertThat(projectColumns, equalTo(expectedOutput)); eval = as(project.child(), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", string("fork5")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", string("fork5")))); limit = as(eval.child(), Limit.class); assertThat(as(limit.limit(), Literal.class).value(), equalTo(9)); filter = as(limit.child(), Filter.class); @@ -3498,7 +3505,7 @@ public void testForkBranchesWithDifferentSchemas() { } eval = as(eval.child(), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", string("fork1")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", string("fork1")))); limit = as(eval.child(), Limit.class); assertThat(as(limit.limit(), Literal.class).value(), equalTo(7)); var orderBy = as(limit.child(), OrderBy.class); @@ -3527,7 +3534,7 @@ public void testForkBranchesWithDifferentSchemas() { } eval = as(eval.child(), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", string("fork2")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", string("fork2")))); eval = as(eval.child(), Eval.class); Alias alias = as(eval.fields().get(0), Alias.class); assertThat(alias.name(), equalTo("xyz")); @@ -3557,7 +3564,7 @@ public void testForkBranchesWithDifferentSchemas() { } eval = as(eval.child(), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", string("fork3")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", string("fork3")))); eval = as(eval.child(), Eval.class); alias = as(eval.fields().get(0), Alias.class); @@ -3984,7 +3991,7 @@ public void testResolveRerankFields() { assertThat(rerank.queryText(), equalTo(string("italian food recipe"))); assertThat(rerank.inferenceId(), equalTo(string("reranking-inference-id"))); - assertThat(rerank.rerankFields(), equalTo(List.of(alias("title", titleAttribute)))); + assertThat(rerank.rerankFields(), equalToIgnoringIds(List.of(alias("title", titleAttribute)))); assertThat(rerank.scoreAttribute(), equalTo(getAttributeByName(relation.output(), MetadataAttribute.SCORE))); } @@ -4008,7 +4015,7 @@ public void testResolveRerankFields() { assertThat(rerank.rerankFields(), hasSize(3)); Attribute titleAttribute = getAttributeByName(relation.output(), "title"); assertThat(titleAttribute, notNullValue()); - assertThat(rerank.rerankFields().get(0), equalTo(alias("title", titleAttribute))); + assertThat(rerank.rerankFields().get(0), equalToIgnoringIds(alias("title", titleAttribute))); Attribute descriptionAttribute = getAttributeByName(relation.output(), "description"); assertThat(descriptionAttribute, notNullValue()); @@ -4021,7 +4028,7 @@ public void testResolveRerankFields() { Attribute yearAttribute = getAttributeByName(relation.output(), "year"); assertThat(yearAttribute, notNullValue()); - assertThat(rerank.rerankFields().get(2), equalTo(alias("yearRenamed", yearAttribute))); + assertThat(rerank.rerankFields().get(2), equalToIgnoringIds(alias("yearRenamed", yearAttribute))); assertThat(rerank.scoreAttribute(), equalTo(getAttributeByName(relation.output(), MetadataAttribute.SCORE))); } @@ -4072,7 +4079,7 @@ public void testResolveRerankScoreField() { EsRelation relation = as(filter.child(), EsRelation.class); assertThat(relation.output().stream().noneMatch(attr -> attr.name().equals(MetadataAttribute.SCORE)), is(true)); - assertThat(rerank.scoreAttribute(), equalTo(MetadataAttribute.create(EMPTY, MetadataAttribute.SCORE))); + assertThat(rerank.scoreAttribute(), equalToIgnoringIds(MetadataAttribute.create(EMPTY, MetadataAttribute.SCORE))); assertThat(rerank.output(), hasItem(rerank.scoreAttribute())); } @@ -4171,12 +4178,12 @@ public void testRerankFieldValidTypes() { EsRelation relation = as(rerank.child(), EsRelation.class); Attribute fieldAttribute = getAttributeByName(relation.output(), fieldName); if (DataType.isString(fieldAttribute.dataType())) { - assertThat(rerank.rerankFields(), equalTo(List.of(alias(fieldName, fieldAttribute)))); + assertThat(rerank.rerankFields(), equalToIgnoringIds(List.of(alias(fieldName, fieldAttribute)))); } else { assertThat( rerank.rerankFields(), - equalTo(List.of(alias(fieldName, new ToString(fieldAttribute.source(), fieldAttribute)))) + equalToIgnoringIds(List.of(alias(fieldName, new ToString(fieldAttribute.source(), fieldAttribute)))) ); } } @@ -4234,7 +4241,7 @@ public void testResolveCompletionTargetField() { """, "mapping-books.json"); Completion completion = as(as(plan, Limit.class).child(), Completion.class); - assertThat(completion.targetField(), equalTo(referenceAttribute("translation", DataType.KEYWORD))); + assertThat(completion.targetField(), equalToIgnoringIds(referenceAttribute("translation", DataType.KEYWORD))); } public void testResolveCompletionDefaultTargetField() { @@ -4244,7 +4251,7 @@ public void testResolveCompletionDefaultTargetField() { """, "mapping-books.json"); Completion completion = as(as(plan, Limit.class).child(), Completion.class); - assertThat(completion.targetField(), equalTo(referenceAttribute("completion", DataType.KEYWORD))); + assertThat(completion.targetField(), equalToIgnoringIds(referenceAttribute("completion", DataType.KEYWORD))); } public void testResolveCompletionPrompt() { @@ -4277,7 +4284,7 @@ public void testResolveCompletionOutputFieldOverwriteInputField() { """, "mapping-books.json"); Completion completion = as(as(plan, Limit.class).child(), Completion.class); - assertThat(completion.targetField(), equalTo(referenceAttribute("description", DataType.KEYWORD))); + assertThat(completion.targetField(), equalToIgnoringIds(referenceAttribute("description", DataType.KEYWORD))); EsRelation esRelation = as(completion.child(), EsRelation.class); assertThat(getAttributeByName(completion.output(), "description"), equalTo(completion.targetField())); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/AbstractNamedExpressionSerializationTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/AbstractNamedExpressionSerializationTests.java new file mode 100644 index 0000000000000..8e74046895790 --- /dev/null +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/AbstractNamedExpressionSerializationTests.java @@ -0,0 +1,38 @@ +/* + * 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.expression; + +import org.elasticsearch.xpack.esql.core.expression.NamedExpression; + +public abstract class AbstractNamedExpressionSerializationTests extends AbstractExpressionSerializationTests { + public void testEqualsAndHashCodeIgnoringId() throws Exception { + T instance = createTestInstance(); + T withNewId = mutateNameId(instance); + + assertTrue(instance.equals(withNewId, true)); + assertEquals(instance.hashCode(true), withNewId.hashCode(true)); + + assertEquals(instance.equals(withNewId), instance.equals(withNewId, false)); + } + + public void testEqualsAndHashCodeWithId() throws Exception { + T instance = createTestInstance(); + T withNewId = mutateNameId(instance); + + if (equalityIgnoresId()) { + assertEquals(instance, withNewId); + assertEquals(instance.hashCode(), withNewId.hashCode()); + } else { + assertNotEquals(instance, withNewId); + } + } + + protected abstract T mutateNameId(T instance); + + protected abstract boolean equalityIgnoresId(); +} diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/AliasTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/AliasTests.java index 7bb8ab3e147e2..485a8e394d4cb 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/AliasTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/AliasTests.java @@ -7,24 +7,16 @@ package org.elasticsearch.xpack.esql.expression; -import org.elasticsearch.TransportVersion; -import org.elasticsearch.common.io.stream.NamedWriteableRegistry; -import org.elasticsearch.test.AbstractWireTestCase; import org.elasticsearch.xpack.esql.core.expression.Alias; import org.elasticsearch.xpack.esql.core.expression.Expression; import org.elasticsearch.xpack.esql.core.expression.NameId; -import org.elasticsearch.xpack.esql.core.expression.NamedExpression; import org.elasticsearch.xpack.esql.core.tree.Source; import org.elasticsearch.xpack.esql.core.tree.SourceTests; import org.elasticsearch.xpack.esql.expression.function.ReferenceAttributeTests; -import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput; -import org.elasticsearch.xpack.esql.io.stream.PlanStreamOutput; import java.io.IOException; -import static org.hamcrest.Matchers.equalTo; - -public class AliasTests extends AbstractWireTestCase { +public class AliasTests extends AbstractNamedExpressionSerializationTests { public static Alias randomAlias() { Source source = SourceTests.randomSource(); String name = randomAlphaOfLength(5); @@ -54,23 +46,17 @@ protected Alias mutateInstance(Alias instance) throws IOException { } @Override - protected Alias copyInstance(Alias instance, TransportVersion version) throws IOException { - return copyInstance( - instance, - getNamedWriteableRegistry(), - (out, v) -> new PlanStreamOutput(out, null).writeNamedWriteable(v), - in -> { - PlanStreamInput pin = new PlanStreamInput(in, in.namedWriteableRegistry(), null); - Alias deser = (Alias) pin.readNamedWriteable(NamedExpression.class); - assertThat(deser.id(), equalTo(pin.mapNameId(Long.parseLong(instance.id().toString())))); - return deser; - }, - version - ); + protected boolean alwaysEmptySource() { + return true; + } + + @Override + protected Alias mutateNameId(Alias instance) { + return instance.withId(new NameId()); } @Override - protected final NamedWriteableRegistry getNamedWriteableRegistry() { - return new NamedWriteableRegistry(ExpressionWritables.allExpressions()); + protected boolean equalityIgnoresId() { + return false; } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/UnresolvedAttributeTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/UnresolvedAttributeTests.java new file mode 100644 index 0000000000000..a899f067966d2 --- /dev/null +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/UnresolvedAttributeTests.java @@ -0,0 +1,111 @@ +/* + * 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.expression; + +import org.elasticsearch.TransportVersion; +import org.elasticsearch.xpack.esql.core.expression.NameId; +import org.elasticsearch.xpack.esql.core.expression.UnresolvedAttribute; +import org.elasticsearch.xpack.esql.core.tree.Source; +import org.elasticsearch.xpack.esql.core.tree.SourceTests; + +import java.io.IOException; +import java.util.Objects; + +public class UnresolvedAttributeTests extends AbstractNamedExpressionSerializationTests { + public static UnresolvedAttribute randomUnresolvedAttribute() { + Source source = SourceTests.randomSource(); + String qualifier = randomBoolean() ? null : randomAlphaOfLength(5); + String name = randomAlphaOfLength(5); + NameId id = randomBoolean() ? null : new NameId(); + String unresolvedMessage = randomUnresolvedMessage(); + Object resolutionMetadata = new Object(); + return new UnresolvedAttribute(source, qualifier, name, id, unresolvedMessage, resolutionMetadata); + } + + /** + * A random qualifier. It is important that this be distinct + * from the name and the qualifier for testing transform. + */ + private static String randomUnresolvedMessage() { + return randomAlphaOfLength(7); + } + + @Override + protected UnresolvedAttribute createTestInstance() { + return randomUnresolvedAttribute(); + } + + @Override + protected UnresolvedAttribute mutateInstance(UnresolvedAttribute instance) { + Source source = instance.source(); + String name = instance.name(); + String qualifier = instance.qualifier(); + NameId id = instance.id(); + String unresolvedMessage = instance.unresolvedMessage(); + Object resolutionMetadata = instance.resolutionMetadata(); + + switch (between(0, 4)) { + case 0 -> name = randomValueOtherThan(name, () -> randomBoolean() ? null : randomAlphaOfLength(5)); + case 1 -> qualifier = randomAlphaOfLength(qualifier == null ? 3 : qualifier.length() + 1); + case 2 -> id = new NameId(); + case 3 -> unresolvedMessage = randomValueOtherThan(unresolvedMessage, UnresolvedAttributeTests::randomUnresolvedMessage); + case 4 -> resolutionMetadata = new Object(); + } + return new UnresolvedAttribute(source, qualifier, name, id, unresolvedMessage, resolutionMetadata); + } + + @Override + protected UnresolvedAttribute copyInstance(UnresolvedAttribute instance, TransportVersion version) throws IOException { + // Doesn't escape the node + return new UnresolvedAttribute( + instance.source(), + instance.qualifier(), + instance.name(), + instance.id(), + instance.unresolvedMessage(), + instance.resolutionMetadata() + ); + } + + @Override + protected UnresolvedAttribute mutateNameId(UnresolvedAttribute instance) { + return instance.withId(new NameId()); + } + + @Override + protected boolean equalityIgnoresId() { + return false; + } + + public void testTransform() { + UnresolvedAttribute a = randomUnresolvedAttribute(); + + String newName = randomValueOtherThan(a.name(), () -> randomAlphaOfLength(5)); + assertEquals( + new UnresolvedAttribute(a.source(), a.qualifier(), newName, a.id(), a.unresolvedMessage(), a.resolutionMetadata()), + a.transformPropertiesOnly(Object.class, v -> Objects.equals(v, a.name()) ? newName : v) + ); + + NameId newId = new NameId(); + assertEquals( + new UnresolvedAttribute(a.source(), a.qualifier(), a.name(), newId, a.unresolvedMessage(), a.resolutionMetadata()), + a.transformPropertiesOnly(Object.class, v -> Objects.equals(v, a.id()) ? newId : v) + ); + + String newMessage = randomValueOtherThan(a.unresolvedMessage(), UnresolvedAttributeTests::randomUnresolvedMessage); + assertEquals( + new UnresolvedAttribute(a.source(), a.qualifier(), a.name(), a.id(), newMessage, a.resolutionMetadata()), + a.transformPropertiesOnly(Object.class, v -> Objects.equals(v, a.unresolvedMessage()) ? newMessage : v) + ); + + Object newMeta = new Object(); + assertEquals( + new UnresolvedAttribute(a.source(), a.qualifier(), a.name(), a.id(), a.unresolvedMessage(), newMeta), + a.transformPropertiesOnly(Object.class, v -> Objects.equals(v, a.resolutionMetadata()) ? newMeta : v) + ); + } +} diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/UnresolvedNamePatternTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/UnresolvedNamePatternTests.java new file mode 100644 index 0000000000000..b47baad5bf72c --- /dev/null +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/UnresolvedNamePatternTests.java @@ -0,0 +1,50 @@ +/* + * 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.expression; + +import org.elasticsearch.TransportVersion; +import org.elasticsearch.xpack.esql.core.tree.Source; + +import java.io.IOException; + +public class UnresolvedNamePatternTests extends AbstractNamedExpressionSerializationTests { + @Override + protected UnresolvedNamePattern createTestInstance() { + // No automaton, this is normally injected during parsing and is derived from the pattern. + return new UnresolvedNamePattern(Source.EMPTY, null, randomAlphaOfLength(3), randomAlphaOfLength(3)); + } + + @Override + protected UnresolvedNamePattern mutateInstance(UnresolvedNamePattern instance) { + Source source = instance.source(); + String name = instance.name(); + String pattern = instance.pattern(); + switch (between(0, 1)) { + case 0 -> name = randomValueOtherThan(name, () -> randomAlphaOfLength(4)); + case 1 -> pattern = randomValueOtherThan(pattern, () -> randomAlphaOfLength(4)); + } + return new UnresolvedNamePattern(source, null, name, pattern); + } + + @Override + protected UnresolvedNamePattern mutateNameId(UnresolvedNamePattern instance) { + // Creating a new instance is enough as the NameId is generated automatically. + return new UnresolvedNamePattern(instance.source(), null, instance.pattern(), instance.name()); + } + + @Override + protected boolean equalityIgnoresId() { + return true; + } + + @Override + protected UnresolvedNamePattern copyInstance(UnresolvedNamePattern instance, TransportVersion version) throws IOException { + // Doesn't escape the node + return new UnresolvedNamePattern(instance.source(), null, instance.pattern(), instance.name()); + } +} diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/UnresolvedStarTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/UnresolvedStarTests.java new file mode 100644 index 0000000000000..203dcc6e0be85 --- /dev/null +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/UnresolvedStarTests.java @@ -0,0 +1,47 @@ +/* + * 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.expression; + +import org.elasticsearch.TransportVersion; +import org.elasticsearch.xpack.esql.core.expression.UnresolvedAttribute; +import org.elasticsearch.xpack.esql.core.expression.UnresolvedStar; +import org.elasticsearch.xpack.esql.core.tree.Source; + +import java.io.IOException; + +public class UnresolvedStarTests extends AbstractNamedExpressionSerializationTests { + @Override + protected UnresolvedStar createTestInstance() { + return new UnresolvedStar(Source.EMPTY, randomBoolean() ? null : new UnresolvedAttribute(Source.EMPTY, randomAlphaOfLength(3))); + } + + @Override + protected UnresolvedStar mutateInstance(UnresolvedStar instance) { + Source source = instance.source(); + String qualifier = instance.qualifier() == null ? "" : instance.qualifier().name(); + qualifier = randomValueOtherThan(qualifier, randomAlphaOfLength(4)::toString); + return new UnresolvedStar(source, new UnresolvedAttribute(Source.EMPTY, qualifier)); + } + + @Override + protected UnresolvedStar mutateNameId(UnresolvedStar instance) { + // Creating a new instance is enough as the NameId is generated automatically. + return new UnresolvedStar(instance.source(), instance.qualifier()); + } + + @Override + protected boolean equalityIgnoresId() { + return true; + } + + @Override + protected UnresolvedStar copyInstance(UnresolvedStar instance, TransportVersion version) throws IOException { + // Doesn't escape the node + return new UnresolvedStar(instance.source(), instance.qualifier()); + } +} diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAttributeTestCase.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAttributeTestCase.java deleted file mode 100644 index d59e309790ad2..0000000000000 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAttributeTestCase.java +++ /dev/null @@ -1,121 +0,0 @@ -/* - * 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.expression.function; - -import org.elasticsearch.common.io.stream.NamedWriteableRegistry; -import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.test.AbstractWireSerializingTestCase; -import org.elasticsearch.xpack.esql.EsqlTestUtils; -import org.elasticsearch.xpack.esql.core.expression.Attribute; -import org.elasticsearch.xpack.esql.core.expression.FieldAttribute; -import org.elasticsearch.xpack.esql.core.tree.Source; -import org.elasticsearch.xpack.esql.expression.ExpressionWritables; -import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput; -import org.elasticsearch.xpack.esql.io.stream.PlanStreamOutput; -import org.elasticsearch.xpack.esql.session.Configuration; - -import java.io.IOException; -import java.util.Objects; - -import static org.hamcrest.Matchers.sameInstance; - -public abstract class AbstractAttributeTestCase extends AbstractWireSerializingTestCase< - AbstractAttributeTestCase.ExtraAttribute> { - - /** - * We use a single random config for all serialization because it's pretty - * heavy to build, especially in {@link #testConcurrentSerialization()}. - */ - private Configuration config; - - protected abstract T create(); - - protected abstract T mutate(T instance); - - @Override - protected final ExtraAttribute createTestInstance() { - return new ExtraAttribute(create()); - } - - @Override - @SuppressWarnings("unchecked") - protected final ExtraAttribute mutateInstance(ExtraAttribute instance) { - return new ExtraAttribute(mutate((T) instance.a)); - } - - @Override - protected final NamedWriteableRegistry getNamedWriteableRegistry() { - return new NamedWriteableRegistry(ExpressionWritables.attributes()); - } - - @Override - protected final Writeable.Reader instanceReader() { - return in -> { - PlanStreamInput pin = new PlanStreamInput(in, in.namedWriteableRegistry(), config); - pin.setTransportVersion(in.getTransportVersion()); - return new ExtraAttribute(pin); - }; - } - - /** - * Adds extra equality comparisons needed for testing round trips of {@link Attribute}. - */ - public static class ExtraAttribute implements Writeable { - private final Attribute a; - - ExtraAttribute(Attribute a) { - this.a = a; - assertThat(a.source(), sameInstance(Source.EMPTY)); - } - - ExtraAttribute(PlanStreamInput in) throws IOException { - a = in.readNamedWriteable(Attribute.class); - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - new PlanStreamOutput(out, EsqlTestUtils.TEST_CFG).writeNamedWriteable(a); - } - - @Override - public boolean equals(Object obj) { - if (obj == null) { - return a.equals(null); - } - if (obj.getClass() != getClass()) { - return a.equals(obj); - } - ExtraAttribute other = (ExtraAttribute) obj; - if (false == a.equals(other.a)) { - return false; - } - if (a instanceof FieldAttribute fa && false == fa.field().equals(((FieldAttribute) other.a).field())) { - return false; - } - return a.source() == Source.EMPTY; - } - - @Override - public int hashCode() { - if (a instanceof FieldAttribute fa) { - return Objects.hash(a, a.source(), fa.field()); - } - return Objects.hash(a, a.source()); - } - - @Override - public String toString() { - StringBuilder b = new StringBuilder(a.toString()); - if (a instanceof FieldAttribute fa) { - b.append(", field=").append(fa.field()); - } - return b.toString(); - } - } -} diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/FieldAttributeTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/FieldAttributeTests.java index 0ac170f6c3cab..56d9667f204df 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/FieldAttributeTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/FieldAttributeTests.java @@ -13,9 +13,10 @@ import org.elasticsearch.xpack.esql.core.tree.Source; import org.elasticsearch.xpack.esql.core.type.DataType; import org.elasticsearch.xpack.esql.core.type.EsField; +import org.elasticsearch.xpack.esql.expression.AbstractNamedExpressionSerializationTests; import org.elasticsearch.xpack.esql.type.AbstractEsFieldTypeTests; -public class FieldAttributeTests extends AbstractAttributeTestCase { +public class FieldAttributeTests extends AbstractNamedExpressionSerializationTests { public static FieldAttribute createFieldAttribute(int maxDepth, boolean onlyRepresentable) { Source source = Source.EMPTY; String parentName = maxDepth == 0 || randomBoolean() ? null : randomAlphaOfLength(3); @@ -35,27 +36,39 @@ private static EsField randomRepresentableEsField(int maxDepth) { } @Override - protected FieldAttribute create() { + protected FieldAttribute createTestInstance() { return createFieldAttribute(3, false); } @Override - protected FieldAttribute mutate(FieldAttribute instance) { + protected FieldAttribute mutateInstance(FieldAttribute instance) { Source source = instance.source(); String parentName = instance.parentName(); String name = instance.name(); String qualifier = instance.qualifier(); EsField field = instance.field(); Nullability nullability = instance.nullable(); + NameId id = instance.id(); boolean synthetic = instance.synthetic(); - switch (between(0, 5)) { + switch (between(0, 6)) { case 0 -> parentName = randomValueOtherThan(parentName, () -> randomBoolean() ? null : randomAlphaOfLength(2)); case 1 -> qualifier = randomAlphaOfLength(qualifier == null ? 3 : qualifier.length() + 1); case 2 -> name = randomAlphaOfLength(name.length() + 1); case 3 -> field = randomValueOtherThan(field, () -> AbstractEsFieldTypeTests.randomAnyEsField(3)); case 4 -> nullability = randomValueOtherThan(nullability, () -> randomFrom(Nullability.values())); - case 5 -> synthetic = false == synthetic; + case 5 -> id = new NameId(); + case 6 -> synthetic = false == synthetic; } - return new FieldAttribute(source, parentName, qualifier, name, field, nullability, new NameId(), synthetic); + return new FieldAttribute(source, parentName, qualifier, name, field, nullability, id, synthetic); + } + + @Override + protected FieldAttribute mutateNameId(FieldAttribute instance) { + return (FieldAttribute) instance.withId(new NameId()); + } + + @Override + protected boolean equalityIgnoresId() { + return false; } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/MetadataAttributeTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/MetadataAttributeTests.java index c8c7c86c91778..b983856eb50af 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/MetadataAttributeTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/MetadataAttributeTests.java @@ -12,10 +12,11 @@ import org.elasticsearch.xpack.esql.core.expression.Nullability; import org.elasticsearch.xpack.esql.core.tree.Source; import org.elasticsearch.xpack.esql.core.type.DataType; +import org.elasticsearch.xpack.esql.expression.AbstractNamedExpressionSerializationTests; -public class MetadataAttributeTests extends AbstractAttributeTestCase { +public class MetadataAttributeTests extends AbstractNamedExpressionSerializationTests { @Override - protected MetadataAttribute create() { + protected MetadataAttribute createTestInstance() { return randomMetadataAttribute(); } @@ -30,20 +31,32 @@ public static MetadataAttribute randomMetadataAttribute() { } @Override - protected MetadataAttribute mutate(MetadataAttribute instance) { + protected MetadataAttribute mutateInstance(MetadataAttribute instance) { Source source = instance.source(); String name = instance.name(); DataType type = instance.dataType(); Nullability nullability = instance.nullable(); + NameId id = instance.id(); boolean synthetic = instance.synthetic(); boolean searchable = instance.searchable(); - switch (between(0, 4)) { + switch (between(0, 5)) { case 0 -> name = randomAlphaOfLength(name.length() + 1); case 1 -> type = randomValueOtherThan(type, () -> randomFrom(DataType.types())); case 2 -> nullability = randomValueOtherThan(nullability, () -> randomFrom(Nullability.values())); - case 3 -> synthetic = false == synthetic; - case 4 -> searchable = false == searchable; + case 3 -> id = new NameId(); + case 4 -> synthetic = false == synthetic; + case 5 -> searchable = false == searchable; } - return new MetadataAttribute(source, name, type, nullability, new NameId(), synthetic, searchable); + return new MetadataAttribute(source, name, type, nullability, id, synthetic, searchable); + } + + @Override + protected MetadataAttribute mutateNameId(MetadataAttribute instance) { + return (MetadataAttribute) instance.withId(new NameId()); + } + + @Override + protected boolean equalityIgnoresId() { + return false; } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/ReferenceAttributeTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/ReferenceAttributeTests.java index 1883c52ce614e..04d0ca5427117 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/ReferenceAttributeTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/ReferenceAttributeTests.java @@ -12,8 +12,9 @@ import org.elasticsearch.xpack.esql.core.expression.ReferenceAttribute; import org.elasticsearch.xpack.esql.core.tree.Source; import org.elasticsearch.xpack.esql.core.type.DataType; +import org.elasticsearch.xpack.esql.expression.AbstractNamedExpressionSerializationTests; -public class ReferenceAttributeTests extends AbstractAttributeTestCase { +public class ReferenceAttributeTests extends AbstractNamedExpressionSerializationTests { public static ReferenceAttribute randomReferenceAttribute(boolean onlyRepresentable) { Source source = Source.EMPTY; String qualifier = randomBoolean() ? null : randomAlphaOfLength(3); @@ -27,25 +28,37 @@ public static ReferenceAttribute randomReferenceAttribute(boolean onlyRepresenta } @Override - protected ReferenceAttribute create() { + protected ReferenceAttribute createTestInstance() { return randomReferenceAttribute(false); } @Override - protected ReferenceAttribute mutate(ReferenceAttribute instance) { + protected ReferenceAttribute mutateInstance(ReferenceAttribute instance) { Source source = instance.source(); String qualifier = instance.qualifier(); String name = instance.name(); DataType type = instance.dataType(); Nullability nullability = instance.nullable(); + NameId id = instance.id(); boolean synthetic = instance.synthetic(); - switch (between(0, 4)) { + switch (between(0, 5)) { case 0 -> qualifier = randomAlphaOfLength(qualifier == null ? 3 : qualifier.length() + 1); case 1 -> name = randomAlphaOfLength(name.length() + 1); case 2 -> type = randomValueOtherThan(type, () -> randomFrom(DataType.types())); case 3 -> nullability = randomValueOtherThan(nullability, () -> randomFrom(Nullability.values())); - case 4 -> synthetic = false == synthetic; + case 4 -> id = new NameId(); + case 5 -> synthetic = false == synthetic; } - return new ReferenceAttribute(source, qualifier, name, type, nullability, new NameId(), synthetic); + return new ReferenceAttribute(source, qualifier, name, type, nullability, id, synthetic); + } + + @Override + protected ReferenceAttribute mutateNameId(ReferenceAttribute instance) { + return (ReferenceAttribute) instance.withId(new NameId()); + } + + @Override + protected boolean equalityIgnoresId() { + return false; } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/UnresolvedFunctionTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/UnresolvedFunctionTests.java index 7cb547876e532..efd22d9ed6eb3 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/UnresolvedFunctionTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/UnresolvedFunctionTests.java @@ -17,8 +17,8 @@ import static java.util.Arrays.asList; import static java.util.Collections.singletonList; -import static org.elasticsearch.xpack.esql.core.expression.UnresolvedAttributeTests.randomUnresolvedAttribute; import static org.elasticsearch.xpack.esql.core.tree.SourceTests.randomSource; +import static org.elasticsearch.xpack.esql.expression.UnresolvedAttributeTests.randomUnresolvedAttribute; public class UnresolvedFunctionTests extends AbstractNodeTestCase { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/UnsupportedAttributeTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/UnsupportedAttributeTests.java index e02a04b90dfd2..370f794edcc3c 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/UnsupportedAttributeTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/UnsupportedAttributeTests.java @@ -10,11 +10,12 @@ import org.elasticsearch.xpack.esql.core.expression.NameId; import org.elasticsearch.xpack.esql.core.tree.Source; import org.elasticsearch.xpack.esql.core.type.UnsupportedEsField; +import org.elasticsearch.xpack.esql.expression.AbstractNamedExpressionSerializationTests; import org.elasticsearch.xpack.esql.type.UnsupportedEsFieldTests; -public class UnsupportedAttributeTests extends AbstractAttributeTestCase { +public class UnsupportedAttributeTests extends AbstractNamedExpressionSerializationTests { @Override - protected UnsupportedAttribute create() { + protected UnsupportedAttribute createTestInstance() { return randomUnsupportedAttribute(); } @@ -28,19 +29,30 @@ public static UnsupportedAttribute randomUnsupportedAttribute() { } @Override - protected UnsupportedAttribute mutate(UnsupportedAttribute instance) { + protected UnsupportedAttribute mutateInstance(UnsupportedAttribute instance) { Source source = instance.source(); String qualifier = instance.qualifier(); String name = instance.name(); UnsupportedEsField field = instance.field(); String customMessage = instance.hasCustomMessage() ? instance.unresolvedMessage() : null; - switch (between(0, 3)) { + NameId id = instance.id(); + switch (between(0, 4)) { case 0 -> qualifier = randomAlphaOfLength(qualifier == null ? 3 : qualifier.length() + 1); case 1 -> name = randomAlphaOfLength(name.length() + 1); case 2 -> field = randomValueOtherThan(field, () -> UnsupportedEsFieldTests.randomUnsupportedEsField(4)); case 3 -> customMessage = randomValueOtherThan(customMessage, () -> randomBoolean() ? null : randomAlphaOfLength(9)); - default -> throw new IllegalArgumentException(); + case 4 -> id = new NameId(); } - return new UnsupportedAttribute(source, qualifier, name, field, customMessage, new NameId()); + return new UnsupportedAttribute(source, qualifier, name, field, customMessage, id); + } + + @Override + protected UnsupportedAttribute mutateNameId(UnsupportedAttribute instance) { + return (UnsupportedAttribute) instance.withId(new NameId()); + } + + @Override + protected boolean equalityIgnoresId() { + return false; } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/AvgSerializationTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/AvgSerializationTests.java index 5d6fa21e20813..d9f30f5dd8b05 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/AvgSerializationTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/AvgSerializationTests.java @@ -10,6 +10,7 @@ import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput; import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.xpack.esql.SerializationTestUtils; import org.elasticsearch.xpack.esql.core.expression.Expression; import org.elasticsearch.xpack.esql.core.tree.NodeInfo; import org.elasticsearch.xpack.esql.core.tree.Source; @@ -80,7 +81,12 @@ public void testSerializeOldAvg() throws IOException { PlanStreamOutput planOut = new PlanStreamOutput(out, configuration()); planOut.writeNamedWriteable(oldAvg); try (StreamInput in = new NamedWriteableAwareStreamInput(out.bytes().streamInput(), getNamedWriteableRegistry())) { - PlanStreamInput planIn = new PlanStreamInput(in, getNamedWriteableRegistry(), configuration()); + PlanStreamInput planIn = new PlanStreamInput( + in, + getNamedWriteableRegistry(), + configuration(), + new SerializationTestUtils.TestNameIdMapper() + ); Avg serialized = (Avg) planIn.readNamedWriteable(categoryClass()); assertThat(serialized.source(), equalTo(oldAvg.source())); assertThat(serialized.field(), equalTo(oldAvg.field())); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/SumSerializationTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/SumSerializationTests.java index f15d8bb96cf24..a30c075efbc1f 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/SumSerializationTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/aggregate/SumSerializationTests.java @@ -10,6 +10,7 @@ import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput; import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.xpack.esql.SerializationTestUtils; import org.elasticsearch.xpack.esql.core.expression.Expression; import org.elasticsearch.xpack.esql.core.tree.NodeInfo; import org.elasticsearch.xpack.esql.core.tree.Source; @@ -80,7 +81,12 @@ public void testSerializeOldSum() throws IOException { PlanStreamOutput planOut = new PlanStreamOutput(out, configuration()); planOut.writeNamedWriteable(oldSum); try (StreamInput in = new NamedWriteableAwareStreamInput(out.bytes().streamInput(), getNamedWriteableRegistry())) { - PlanStreamInput planIn = new PlanStreamInput(in, getNamedWriteableRegistry(), configuration()); + PlanStreamInput planIn = new PlanStreamInput( + in, + getNamedWriteableRegistry(), + configuration(), + new SerializationTestUtils.TestNameIdMapper() + ); Sum serialized = (Sum) planIn.readNamedWriteable(categoryClass()); assertThat(serialized.source(), equalTo(oldSum.source())); assertThat(serialized.field(), equalTo(oldSum.field())); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseExtraTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseExtraTests.java index a7ca2a7a37e19..08c4649b548d4 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseExtraTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseExtraTests.java @@ -35,6 +35,7 @@ import java.util.stream.Stream; import static org.elasticsearch.compute.data.BlockUtils.toJavaObject; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.equalToIgnoringIds; import static org.elasticsearch.xpack.esql.expression.function.AbstractFunctionTestCase.field; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.sameInstance; @@ -51,14 +52,14 @@ public void testElseValueExplicit() { field("first_cond", DataType.BOOLEAN), List.of(field("v", DataType.LONG), field("e", DataType.LONG)) ).children(), - equalTo(List.of(field("first_cond", DataType.BOOLEAN), field("v", DataType.LONG), field("e", DataType.LONG))) + equalToIgnoringIds(List.of(field("first_cond", DataType.BOOLEAN), field("v", DataType.LONG), field("e", DataType.LONG))) ); } public void testElseValueImplied() { assertThat( new Case(Source.synthetic("case"), field("first_cond", DataType.BOOLEAN), List.of(field("v", DataType.LONG))).children(), - equalTo(List.of(field("first_cond", DataType.BOOLEAN), field("v", DataType.LONG))) + equalToIgnoringIds(List.of(field("first_cond", DataType.BOOLEAN), field("v", DataType.LONG))) ); } @@ -71,7 +72,9 @@ public void testPartialFoldDropsFirstFalse() { assertThat(c.foldable(), equalTo(false)); assertThat( c.partiallyFold(FoldContext.small()), - equalTo(new Case(Source.synthetic("case"), field("last_cond", DataType.BOOLEAN), List.of(field("last", DataType.LONG)))) + equalToIgnoringIds( + new Case(Source.synthetic("case"), field("last_cond", DataType.BOOLEAN), List.of(field("last", DataType.LONG))) + ) ); } @@ -84,7 +87,9 @@ public void testPartialFoldMv() { assertThat(c.foldable(), equalTo(false)); assertThat( c.partiallyFold(FoldContext.small()), - equalTo(new Case(Source.synthetic("case"), field("last_cond", DataType.BOOLEAN), List.of(field("last", DataType.LONG)))) + equalToIgnoringIds( + new Case(Source.synthetic("case"), field("last_cond", DataType.BOOLEAN), List.of(field("last", DataType.LONG))) + ) ); } @@ -105,7 +110,7 @@ public void testPartialFoldFirst() { List.of(field("first", DataType.LONG), field("last", DataType.LONG)) ); assertThat(c.foldable(), equalTo(false)); - assertThat(c.partiallyFold(FoldContext.small()), equalTo(field("first", DataType.LONG))); + assertThat(c.partiallyFold(FoldContext.small()), equalToIgnoringIds(field("first", DataType.LONG))); } public void testPartialFoldFirstAfterKeepingUnknown() { @@ -122,7 +127,7 @@ public void testPartialFoldFirstAfterKeepingUnknown() { assertThat(c.foldable(), equalTo(false)); assertThat( c.partiallyFold(FoldContext.small()), - equalTo( + equalToIgnoringIds( new Case( Source.synthetic("case"), field("keep_me_cond", DataType.BOOLEAN), @@ -144,7 +149,7 @@ public void testPartialFoldSecond() { ) ); assertThat(c.foldable(), equalTo(false)); - assertThat(c.partiallyFold(FoldContext.small()), equalTo(field("second", DataType.LONG))); + assertThat(c.partiallyFold(FoldContext.small()), equalToIgnoringIds(field("second", DataType.LONG))); } public void testPartialFoldSecondAfterDroppingFalse() { @@ -159,7 +164,7 @@ public void testPartialFoldSecondAfterDroppingFalse() { ) ); assertThat(c.foldable(), equalTo(false)); - assertThat(c.partiallyFold(FoldContext.small()), equalTo(field("second", DataType.LONG))); + assertThat(c.partiallyFold(FoldContext.small()), equalToIgnoringIds(field("second", DataType.LONG))); } public void testPartialFoldLast() { @@ -174,7 +179,7 @@ public void testPartialFoldLast() { ) ); assertThat(c.foldable(), equalTo(false)); - assertThat(c.partiallyFold(FoldContext.small()), equalTo(field("last", DataType.LONG))); + assertThat(c.partiallyFold(FoldContext.small()), equalToIgnoringIds(field("last", DataType.LONG))); } public void testPartialFoldLastAfterKeepingUnknown() { @@ -191,7 +196,7 @@ public void testPartialFoldLastAfterKeepingUnknown() { assertThat(c.foldable(), equalTo(false)); assertThat( c.partiallyFold(FoldContext.small()), - equalTo( + equalToIgnoringIds( new Case( Source.synthetic("case"), field("keep_me_cond", DataType.BOOLEAN), diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseTests.java index 57470bc26c376..4059ad709d0f5 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseTests.java @@ -31,6 +31,7 @@ import java.util.stream.Stream; import static java.util.Collections.unmodifiableList; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.equalToIgnoringIds; import static org.elasticsearch.xpack.esql.EsqlTestUtils.randomLiteral; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.sameInstance; @@ -808,14 +809,14 @@ public void testPartialFold() { return; } if (extra().expectedPartialFold.size() == 1) { - assertThat(c.partiallyFold(FoldContext.small()), equalTo(extra().expectedPartialFold.get(0).asField())); + assertThat(c.partiallyFold(FoldContext.small()), equalToIgnoringIds(extra().expectedPartialFold.get(0).asField())); return; } Case expected = build( Source.synthetic("expected"), extra().expectedPartialFold.stream().map(TestCaseSupplier.TypedData::asField).toList() ); - assertThat(c.partiallyFold(FoldContext.small()), equalTo(expected)); + assertThat(c.partiallyFold(FoldContext.small()), equalToIgnoringIds(expected)); } private static Function addWarnings(List warnings) { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/io/stream/PlanStreamOutputTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/io/stream/PlanStreamOutputTests.java index bb5f80b18e3cf..04b7bf6a626d0 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/io/stream/PlanStreamOutputTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/io/stream/PlanStreamOutputTests.java @@ -18,6 +18,7 @@ import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.TransportVersionUtils; import org.elasticsearch.xpack.esql.Column; +import org.elasticsearch.xpack.esql.SerializationTestUtils; import org.elasticsearch.xpack.esql.core.InvalidArgumentException; import org.elasticsearch.xpack.esql.core.expression.Attribute; import org.elasticsearch.xpack.esql.core.expression.NameId; @@ -143,7 +144,14 @@ public void testWriteMultipleAttributes() throws IOException { } } - try (PlanStreamInput in = new PlanStreamInput(out.bytes().streamInput(), REGISTRY, configuration)) { + try ( + PlanStreamInput in = new PlanStreamInput( + out.bytes().streamInput(), + REGISTRY, + configuration, + new SerializationTestUtils.TestNameIdMapper() + ) + ) { List readAttrs = new ArrayList<>(); for (int i = 0; i < occurrences; i++) { readAttrs.add(in.readNamedWriteable(Attribute.class)); @@ -181,7 +189,14 @@ public void testWriteEqualAttributesDifferentID() throws IOException { planStream.writeNamedWriteable(one); planStream.writeNamedWriteable(two); - try (PlanStreamInput in = new PlanStreamInput(out.bytes().streamInput(), REGISTRY, configuration)) { + try ( + PlanStreamInput in = new PlanStreamInput( + out.bytes().streamInput(), + REGISTRY, + configuration, + new SerializationTestUtils.TestNameIdMapper() + ) + ) { Attribute oneCopy = in.readNamedWriteable(Attribute.class); Attribute twoCopy = in.readNamedWriteable(Attribute.class); @@ -203,7 +218,14 @@ public void testWriteDifferentAttributesSameID() throws IOException { planStream.writeNamedWriteable(one); planStream.writeNamedWriteable(two); - try (PlanStreamInput in = new PlanStreamInput(out.bytes().streamInput(), REGISTRY, configuration)) { + try ( + PlanStreamInput in = new PlanStreamInput( + out.bytes().streamInput(), + REGISTRY, + configuration, + new SerializationTestUtils.TestNameIdMapper() + ) + ) { Attribute oneCopy = in.readNamedWriteable(Attribute.class); Attribute twoCopy = in.readNamedWriteable(Attribute.class); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/IgnoreNullMetricsPhysicalPlannerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/IgnoreNullMetricsPhysicalPlannerTests.java index c89325d1bbd08..09962b694addd 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/IgnoreNullMetricsPhysicalPlannerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/IgnoreNullMetricsPhysicalPlannerTests.java @@ -15,6 +15,7 @@ import org.elasticsearch.xpack.esql.session.Configuration; import static org.elasticsearch.index.query.QueryBuilders.existsQuery; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.assertEqualsIgnoringIds; import static org.elasticsearch.xpack.esql.core.querydsl.query.Query.unscore; import static org.hamcrest.Matchers.is; @@ -47,7 +48,7 @@ public void testSamePhysicalPlans() { """; PhysicalPlan expectedPlan = plannerOptimizerTimeSeries.plan(controlQuery); - assertEquals(NodeUtils.diffString(expectedPlan, actualPlan), expectedPlan, actualPlan); + assertEqualsIgnoringIds(NodeUtils.diffString(expectedPlan, actualPlan), expectedPlan, actualPlan); } public void testPushdownOfSimpleCounterQuery() { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java index 4d65794f3d7f7..fc9404633f685 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java @@ -161,10 +161,12 @@ import static org.elasticsearch.xpack.esql.EsqlTestUtils.TWO; import static org.elasticsearch.xpack.esql.EsqlTestUtils.as; import static org.elasticsearch.xpack.esql.EsqlTestUtils.asLimit; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.containsIgnoringIds; import static org.elasticsearch.xpack.esql.EsqlTestUtils.emptyInferenceResolution; import static org.elasticsearch.xpack.esql.EsqlTestUtils.emptySource; import static org.elasticsearch.xpack.esql.EsqlTestUtils.fieldAttribute; import static org.elasticsearch.xpack.esql.EsqlTestUtils.getFieldAttribute; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.ignoreIds; import static org.elasticsearch.xpack.esql.EsqlTestUtils.localSource; import static org.elasticsearch.xpack.esql.EsqlTestUtils.randomLiteral; import static org.elasticsearch.xpack.esql.EsqlTestUtils.referenceAttribute; @@ -1238,7 +1240,7 @@ public void testPushDownEvalPastProject() { var eval = as(keep.child(), Eval.class); assertThat( eval.fields(), - contains( + containsIgnoringIds( new Alias( EMPTY, "y", @@ -1258,7 +1260,7 @@ public void testPushDownDissectPastProject() { var keep = as(plan, Project.class); var dissect = as(keep.child(), Dissect.class); - assertThat(dissect.extractedFields(), contains(referenceAttribute("y", DataType.KEYWORD))); + assertThat(dissect.extractedFields(), containsIgnoringIds(referenceAttribute("y", DataType.KEYWORD))); } public void testPushDownGrokPastProject() { @@ -1271,7 +1273,7 @@ public void testPushDownGrokPastProject() { var keep = as(plan, Project.class); var grok = as(keep.child(), Grok.class); - assertThat(grok.extractedFields(), contains(referenceAttribute("y", DataType.KEYWORD))); + assertThat(grok.extractedFields(), containsIgnoringIds(referenceAttribute("y", DataType.KEYWORD))); } public void testPushDownFilterPastProjectUsingEval() { @@ -2705,7 +2707,7 @@ public void testDontPruneSameFieldDifferentDirectionSortClauses() { var topN = as(keep.child(), TopN.class); assertThat( topN.order(), - contains( + containsIgnoringIds( new Order( EMPTY, new ReferenceAttribute(EMPTY, null, "e", INTEGER, Nullability.TRUE, null, false), @@ -2748,7 +2750,7 @@ public void testPruneRedundantSortClauses() { var topN = as(project.child(), TopN.class); assertThat( topN.order(), - contains( + containsIgnoringIds( new Order( EMPTY, new ReferenceAttribute(EMPTY, null, "e", INTEGER, Nullability.TRUE, null, false), @@ -2790,7 +2792,7 @@ public void testDontPruneSameFieldDifferentDirectionSortClauses_UsingAlias() { var topN = as(keep.child(), TopN.class); assertThat( topN.order(), - contains( + containsIgnoringIds( new Order( EMPTY, new FieldAttribute(EMPTY, "emp_no", mapping.get("emp_no")), @@ -2813,7 +2815,7 @@ public void testPruneRedundantSortClausesUsingAlias() { var topN = as(project.child(), TopN.class); assertThat( topN.order(), - contains( + containsIgnoringIds( new Order( EMPTY, new FieldAttribute(EMPTY, "emp_no", mapping.get("emp_no")), @@ -6393,7 +6395,7 @@ public void testInlineStatsWithRow() { var localRelation = as(limit.child(), LocalRelation.class); assertThat( localRelation.output(), - contains( + containsIgnoringIds( new ReferenceAttribute(EMPTY, "salary", INTEGER), new ReferenceAttribute(EMPTY, "emp_no", INTEGER), new ReferenceAttribute(EMPTY, "gender", KEYWORD) @@ -6686,7 +6688,8 @@ private void doTestSimplifyComparisonArithmetics( private void assertSemanticMatching(String expected, String provided) { BinaryComparison bc = extractPlannedBinaryComparison(provided); LogicalPlan exp = analyzerTypes.analyze(parser.createStatement("FROM types | WHERE " + expected)); - assertSemanticMatching(bc, extractPlannedBinaryComparison(exp)); + // Exp is created separately, so the IDs will be different - ignore them for the comparison. + assertSemanticMatching((BinaryComparison) ignoreIds(bc), (EsqlBinaryComparison) ignoreIds(extractPlannedBinaryComparison(exp))); } private static void assertSemanticMatching(Expression fieldAttributeExp, Expression unresolvedAttributeExp) { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/OptimizerRulesTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/OptimizerRulesTests.java index 6a720936c8180..3659b7624af68 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/OptimizerRulesTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/OptimizerRulesTests.java @@ -28,12 +28,12 @@ import java.util.Collections; import java.util.List; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.containsInAnyOrderIgnoringIds; import static org.elasticsearch.xpack.esql.EsqlTestUtils.randomMinimumVersion; import static org.elasticsearch.xpack.esql.EsqlTestUtils.rangeOf; import static org.elasticsearch.xpack.esql.core.type.DataType.BOOLEAN; import static org.elasticsearch.xpack.esql.core.util.TestUtils.getFieldAttribute; import static org.elasticsearch.xpack.esql.core.util.TestUtils.of; -import static org.hamcrest.Matchers.containsInAnyOrder; public class OptimizerRulesTests extends ESTestCase { @@ -139,6 +139,6 @@ protected Expression rule(Expression e, LogicalOptimizerContext ctx) { var alias = new Alias(new Source(1, 18, "x=f1+1"), "x", add); // contains expressions only from EVAL - assertThat(rule.appliedTo, containsInAnyOrder(alias, add, attribute, literal)); + assertThat(rule.appliedTo, containsInAnyOrderIgnoringIds(alias, add, attribute, literal)); } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/AbstractStatementParserTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/AbstractStatementParserTests.java index 462b9f7373ecf..86b594c027a4a 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/AbstractStatementParserTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/AbstractStatementParserTests.java @@ -28,11 +28,11 @@ import java.util.List; import java.util.Map; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.equalToIgnoringIds; import static org.elasticsearch.xpack.esql.core.tree.Source.EMPTY; import static org.elasticsearch.xpack.esql.core.util.NumericUtils.asLongUnsigned; import static org.elasticsearch.xpack.esql.expression.function.FunctionResolutionStrategy.DEFAULT; import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; abstract class AbstractStatementParserTests extends ESTestCase { @@ -45,7 +45,7 @@ void assertStatement(String statement, LogicalPlan expected) { } catch (Exception e) { throw new AssertionError("parsing error for [" + statement + "]", e); } - assertThat(statement, actual, equalTo(expected)); + assertThat(statement, actual, equalToIgnoringIds(expected)); } LogicalPlan statement(String query, String arg) { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/ExpressionTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/ExpressionTests.java index c39dc70f3ad39..163dc02e1b78c 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/ExpressionTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/ExpressionTests.java @@ -44,6 +44,8 @@ import java.util.stream.IntStream; import static org.elasticsearch.xpack.esql.EsqlTestUtils.as; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.assertEqualsIgnoringIds; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.equalToIgnoringIds; import static org.elasticsearch.xpack.esql.core.tree.Source.EMPTY; import static org.elasticsearch.xpack.esql.core.type.DataType.DATE_PERIOD; import static org.elasticsearch.xpack.esql.core.type.DataType.DOUBLE; @@ -326,32 +328,32 @@ public void testOperatorsPrecedenceWithNegation() { } public void testOperatorsPrecedenceExpressionsEquality() { - assertThat(whereExpression("a-1>2 or b>=5 and c-1>=5"), equalTo(whereExpression("((a-1)>2 or (b>=5 and (c-1)>=5))"))); + assertThat(whereExpression("a-1>2 or b>=5 and c-1>=5"), equalToIgnoringIds(whereExpression("((a-1)>2 or (b>=5 and (c-1)>=5))"))); assertThat( whereExpression("a*5==25 and b>5 and c%4>=1 or true or false"), - equalTo(whereExpression("(((((a*5)==25) and (b>5) and ((c%4)>=1)) or true) or false)")) + equalToIgnoringIds(whereExpression("(((((a*5)==25) and (b>5) and ((c%4)>=1)) or true) or false)")) ); assertThat( whereExpression("a*4-b*5<100 and b/2+c*6>=50 or c%5+x>=5"), - equalTo(whereExpression("((((a*4)-(b*5))<100) and (((b/2)+(c*6))>=50)) or (((c%5)+x)>=5)")) + equalToIgnoringIds(whereExpression("((((a*4)-(b*5))<100) and (((b/2)+(c*6))>=50)) or (((c%5)+x)>=5)")) ); assertThat( whereExpression("true and false or true and c/12+x*5-y%2>=50"), - equalTo(whereExpression("((true and false) or (true and (((c/12)+(x*5)-(y%2))>=50)))")) + equalToIgnoringIds(whereExpression("((true and false) or (true and (((c/12)+(x*5)-(y%2))>=50)))")) ); assertThat( whereExpression("10 days > 5 hours and 1/5 minutes > 8 seconds * 3 and -1 minutes > foo"), - equalTo(whereExpression("((10 days) > (5 hours)) and ((1/(5 minutes) > ((8 seconds) * 3))) and (-1 minute > foo)")) + equalToIgnoringIds(whereExpression("((10 days) > (5 hours)) and ((1/(5 minutes) > ((8 seconds) * 3))) and (-1 minute > foo)")) ); assertThat( whereExpression("10 DAYS > 5 HOURS and 1/5 MINUTES > 8 SECONDS * 3 and -1 MINUTES > foo"), - equalTo(whereExpression("((10 days) > (5 hours)) and ((1/(5 minutes) > ((8 seconds) * 3))) and (-1 minute > foo)")) + equalToIgnoringIds(whereExpression("((10 days) > (5 hours)) and ((1/(5 minutes) > ((8 seconds) * 3))) and (-1 minute > foo)")) ); } public void testFunctionExpressions() { assertEquals(new UnresolvedFunction(EMPTY, "fn", DEFAULT, new ArrayList<>()), whereExpression("fn()")); - assertEquals( + assertEqualsIgnoringIds( new UnresolvedFunction( EMPTY, "invoke", @@ -365,13 +367,13 @@ public void testFunctionExpressions() { ), whereExpression("invoke(a, b + c)") ); - assertEquals(whereExpression("(invoke((a + b)))"), whereExpression("invoke(a+b)")); - assertEquals(whereExpression("((fn()) + fn(fn()))"), whereExpression("fn() + fn(fn())")); + assertEqualsIgnoringIds(whereExpression("(invoke((a + b)))"), whereExpression("invoke(a+b)")); + assertEqualsIgnoringIds(whereExpression("((fn()) + fn(fn()))"), whereExpression("fn() + fn(fn())")); } public void testUnquotedIdentifiers() { for (String identifier : List.of("a", "_a", "a_b", "a9", "abc123", "a_____9", "__a_b", "@a", "_1", "@2")) { - assertEquals(new UnresolvedAttribute(EMPTY, identifier), whereExpression(identifier)); + assertEqualsIgnoringIds(new UnresolvedAttribute(EMPTY, identifier), whereExpression(identifier)); } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/QualifierTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/QualifierTests.java index dfc13c7b3f24c..855a798b9fa80 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/QualifierTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/QualifierTests.java @@ -23,6 +23,7 @@ import java.util.function.Function; import static org.elasticsearch.xpack.esql.EsqlTestUtils.as; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.assertEqualsIgnoringIds; public class QualifierTests extends AbstractStatementParserTests { public void testQualifiersAreDisabledInReleaseBuilds() { @@ -57,7 +58,7 @@ public void testSimpleQualifierInExpression() { UnresolvedRelation relation = as(join.right(), UnresolvedRelation.class); UnresolvedAttribute filterExpr = as(filter.condition(), UnresolvedAttribute.class); - assertEquals(new UnresolvedAttribute(Source.EMPTY, "qualified", "field", null), filterExpr); + assertEqualsIgnoringIds(new UnresolvedAttribute(Source.EMPTY, "qualified", "field", null), filterExpr); assertEquals("Unknown column [[qualified].[field]]", filterExpr.unresolvedMessage()); String referenceQuery = "ROW x = 1 | LOOKUP JOIN lu_idx AS qualified ON x | WHERE qualified.field"; @@ -894,7 +895,7 @@ private void assertQualifiedAttributeInExpressions( LogicalPlan referencePlan = statement(referenceQuery).transformExpressionsDown(Expression.class, normalizeExpressions); - assertEquals(referencePlan, planWithStrippedQualifiers); + assertEqualsIgnoringIds(referencePlan, planWithStrippedQualifiers); } private static int countAttribute(Expression expr, String qualifier, String name) { @@ -912,6 +913,6 @@ private static int countAttribute(Expression expr, String qualifier, String name private void assertStatementsEqual(String query1, String query2) { LogicalPlan plan1 = statement(query1); LogicalPlan plan2 = statement(query2); - assertEquals(plan1, plan2); + assertEqualsIgnoringIds(plan1, plan2); } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java index 0a1406dd3bc59..8cfa59cce85d9 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java @@ -83,6 +83,8 @@ import java.util.stream.Stream; import static org.elasticsearch.xpack.esql.EsqlTestUtils.as; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.assertEqualsIgnoringIds; +import static org.elasticsearch.xpack.esql.EsqlTestUtils.equalToIgnoringIds; import static org.elasticsearch.xpack.esql.EsqlTestUtils.paramAsConstant; import static org.elasticsearch.xpack.esql.EsqlTestUtils.paramAsIdentifier; import static org.elasticsearch.xpack.esql.EsqlTestUtils.paramAsPattern; @@ -120,14 +122,14 @@ public class StatementParserTests extends AbstractStatementParserTests { private static final LogicalPlan PROCESSING_CMD_INPUT = new Row(EMPTY, List.of(new Alias(EMPTY, "a", integer(1)))); public void testRowCommand() { - assertEquals( + assertEqualsIgnoringIds( new Row(EMPTY, List.of(new Alias(EMPTY, "a", integer(1)), new Alias(EMPTY, "b", integer(2)))), statement("row a = 1, b = 2") ); } public void testRowCommandImplicitFieldName() { - assertEquals( + assertEqualsIgnoringIds( new Row( EMPTY, List.of(new Alias(EMPTY, "1", integer(1)), new Alias(EMPTY, "2", integer(2)), new Alias(EMPTY, "c", integer(3))) @@ -137,94 +139,106 @@ public void testRowCommandImplicitFieldName() { } public void testRowCommandLong() { - assertEquals(new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalLong(2147483648L)))), statement("row c = 2147483648")); + assertEqualsIgnoringIds(new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalLong(2147483648L)))), statement("row c = 2147483648")); } public void testRowCommandHugeInt() { - assertEquals( + assertEqualsIgnoringIds( new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalUnsignedLong("9223372036854775808")))), statement("row c = 9223372036854775808") ); - assertEquals( + assertEqualsIgnoringIds( new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalDouble(18446744073709551616.)))), statement("row c = 18446744073709551616") ); } public void testRowCommandHugeNegativeInt() { - assertEquals( + assertEqualsIgnoringIds( new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalDouble(-92233720368547758080d)))), statement("row c = -92233720368547758080") ); - assertEquals( + assertEqualsIgnoringIds( new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalDouble(-18446744073709551616d)))), statement("row c = -18446744073709551616") ); } public void testRowCommandDouble() { - assertEquals(new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalDouble(1.0)))), statement("row c = 1.0")); + assertEqualsIgnoringIds(new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalDouble(1.0)))), statement("row c = 1.0")); } public void testRowCommandMultivalueInt() { - assertEquals(new Row(EMPTY, List.of(new Alias(EMPTY, "c", integers(1, 2, -5)))), statement("row c = [1, 2, -5]")); + assertEqualsIgnoringIds(new Row(EMPTY, List.of(new Alias(EMPTY, "c", integers(1, 2, -5)))), statement("row c = [1, 2, -5]")); } public void testRowCommandMultivalueLong() { - assertEquals( + assertEqualsIgnoringIds( new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalLongs(2147483648L, 2147483649L, -434366649L)))), statement("row c = [2147483648, 2147483649, -434366649]") ); } public void testRowCommandMultivalueLongAndInt() { - assertEquals(new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalLongs(2147483648L, 1L)))), statement("row c = [2147483648, 1]")); + assertEqualsIgnoringIds( + new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalLongs(2147483648L, 1L)))), + statement("row c = [2147483648, 1]") + ); } public void testRowCommandMultivalueHugeInts() { - assertEquals( + assertEqualsIgnoringIds( new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalDoubles(18446744073709551616., 18446744073709551617.)))), statement("row c = [18446744073709551616, 18446744073709551617]") ); - assertEquals( + assertEqualsIgnoringIds( new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalUnsignedLongs("9223372036854775808", "9223372036854775809")))), statement("row c = [9223372036854775808, 9223372036854775809]") ); } public void testRowCommandMultivalueHugeIntAndNormalInt() { - assertEquals( + assertEqualsIgnoringIds( new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalDoubles(18446744073709551616., 1.0)))), statement("row c = [18446744073709551616, 1]") ); - assertEquals( + assertEqualsIgnoringIds( new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalUnsignedLongs("9223372036854775808", "1")))), statement("row c = [9223372036854775808, 1]") ); } public void testRowCommandMultivalueDouble() { - assertEquals(new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalDoubles(1.0, 2.0, -3.4)))), statement("row c = [1.0, 2.0, -3.4]")); + assertEqualsIgnoringIds( + new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalDoubles(1.0, 2.0, -3.4)))), + statement("row c = [1.0, 2.0, -3.4]") + ); } public void testRowCommandBoolean() { - assertEquals(new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalBoolean(false)))), statement("row c = false")); + assertEqualsIgnoringIds(new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalBoolean(false)))), statement("row c = false")); } public void testRowCommandMultivalueBoolean() { - assertEquals(new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalBooleans(false, true)))), statement("row c = [false, true]")); + assertEqualsIgnoringIds( + new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalBooleans(false, true)))), + statement("row c = [false, true]") + ); } public void testRowCommandString() { - assertEquals(new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalString("chicken")))), statement("row c = \"chicken\"")); + assertEqualsIgnoringIds(new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalString("chicken")))), statement("row c = \"chicken\"")); } public void testRowCommandMultivalueString() { - assertEquals(new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalStrings("cat", "dog")))), statement("row c = [\"cat\", \"dog\"]")); + assertEqualsIgnoringIds( + new Row(EMPTY, List.of(new Alias(EMPTY, "c", literalStrings("cat", "dog")))), + statement("row c = [\"cat\", \"dog\"]") + ); } public void testRowCommandWithEscapedFieldName() { - assertEquals( + assertEqualsIgnoringIds( new Row( EMPTY, List.of( @@ -238,14 +252,14 @@ public void testRowCommandWithEscapedFieldName() { } public void testCompositeCommand() { - assertEquals( + assertEqualsIgnoringIds( new Filter(EMPTY, new Row(EMPTY, List.of(new Alias(EMPTY, "a", integer(1)))), TRUE), statement("row a = 1 | where true") ); } public void testMultipleCompositeCommands() { - assertEquals( + assertEqualsIgnoringIds( new Filter( EMPTY, new Filter(EMPTY, new Filter(EMPTY, new Row(EMPTY, List.of(new Alias(EMPTY, "a", integer(1)))), TRUE), FALSE), @@ -256,12 +270,12 @@ public void testMultipleCompositeCommands() { } public void testEval() { - assertEquals( + assertEqualsIgnoringIds( new Eval(EMPTY, PROCESSING_CMD_INPUT, List.of(new Alias(EMPTY, "b", attribute("a")))), processingCommand("eval b = a") ); - assertEquals( + assertEqualsIgnoringIds( new Eval( EMPTY, PROCESSING_CMD_INPUT, @@ -272,9 +286,12 @@ public void testEval() { } public void testEvalImplicitNames() { - assertEquals(new Eval(EMPTY, PROCESSING_CMD_INPUT, List.of(new Alias(EMPTY, "a", attribute("a")))), processingCommand("eval a")); + assertEqualsIgnoringIds( + new Eval(EMPTY, PROCESSING_CMD_INPUT, List.of(new Alias(EMPTY, "a", attribute("a")))), + processingCommand("eval a") + ); - assertEquals( + assertEqualsIgnoringIds( new Eval( EMPTY, PROCESSING_CMD_INPUT, @@ -291,7 +308,7 @@ public void testEvalImplicitNames() { } public void testStatsWithGroups() { - assertEquals( + assertEqualsIgnoringIds( new Aggregate( EMPTY, PROCESSING_CMD_INPUT, @@ -307,7 +324,7 @@ public void testStatsWithGroups() { } public void testStatsWithoutGroups() { - assertEquals( + assertEqualsIgnoringIds( new Aggregate( EMPTY, PROCESSING_CMD_INPUT, @@ -322,7 +339,7 @@ public void testStatsWithoutGroups() { } public void testStatsWithoutAggs() { - assertEquals( + assertEqualsIgnoringIds( new Aggregate(EMPTY, PROCESSING_CMD_INPUT, List.of(attribute("a")), List.of(attribute("a"))), processingCommand("stats by a") ); @@ -357,7 +374,7 @@ public void testStatsWithGroupKeyAndAggFilter() { var a = attribute("a"); var f = new UnresolvedFunction(EMPTY, "min", DEFAULT, List.of(a)); var filter = new Alias(EMPTY, "min(a) where a > 1", new FilteredExpression(EMPTY, f, new GreaterThan(EMPTY, a, integer(1)))); - assertEquals( + assertEqualsIgnoringIds( new Aggregate(EMPTY, PROCESSING_CMD_INPUT, List.of(a), List.of(filter, a)), processingCommand("stats min(a) where a > 1 by a") ); @@ -380,7 +397,7 @@ public void testStatsWithGroupKeyAndMixedAggAndFilter() { var avg_filter_ex = new GreaterThan(EMPTY, new Div(EMPTY, a, integer(2)), integer(100)); var avg_filter = new Alias(EMPTY, "avg", new FilteredExpression(EMPTY, avg, avg_filter_ex)); - assertEquals( + assertEqualsIgnoringIds( new Aggregate(EMPTY, PROCESSING_CMD_INPUT, List.of(a), List.of(min_alias, max_filter, avg_filter, a)), processingCommand(""" stats @@ -396,7 +413,10 @@ public void testStatsWithoutGroupKeyMixedAggAndFilter() { var a = attribute("a"); var f = new UnresolvedFunction(EMPTY, "min", DEFAULT, List.of(a)); var filter = new Alias(EMPTY, "min(a) where a > 1", new FilteredExpression(EMPTY, f, new GreaterThan(EMPTY, a, integer(1)))); - assertEquals(new Aggregate(EMPTY, PROCESSING_CMD_INPUT, List.of(), List.of(filter)), processingCommand("stats min(a) where a > 1")); + assertEqualsIgnoringIds( + new Aggregate(EMPTY, PROCESSING_CMD_INPUT, List.of(), List.of(filter)), + processingCommand("stats min(a) where a > 1") + ); } public void testInlineStatsWithGroups() { @@ -407,7 +427,7 @@ public void testInlineStatsWithGroups() { var query = cmd + " b = MIN(a) BY c, d.e"; assertThat( processingCommand(query), - is( + equalToIgnoringIds( new InlineStats( EMPTY, new Aggregate( @@ -432,19 +452,17 @@ public void testInlineStatsWithoutGroups() { } for (var cmd : List.of("INLINE STATS", "INLINESTATS")) { var query = cmd + " MIN(a), c = 1"; - assertThat( + assertEqualsIgnoringIds( processingCommand(query), - is( - new InlineStats( + new InlineStats( + EMPTY, + new Aggregate( EMPTY, - new Aggregate( - EMPTY, - PROCESSING_CMD_INPUT, - List.of(), - List.of( - new Alias(EMPTY, "MIN(a)", new UnresolvedFunction(EMPTY, "MIN", DEFAULT, List.of(attribute("a")))), - new Alias(EMPTY, "c", integer(1)) - ) + PROCESSING_CMD_INPUT, + List.of(), + List.of( + new Alias(EMPTY, "MIN(a)", new UnresolvedFunction(EMPTY, "MIN", DEFAULT, List.of(attribute("a")))), + new Alias(EMPTY, "c", integer(1)) ) ) ) @@ -503,7 +521,7 @@ public void testInlineStatsWithinFork() { // first subplan var eval = as(subPlans.get(0), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", literalString("fork1")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", literalString("fork1")))); var limit = as(eval.child(), Limit.class); assertThat(limit.limit(), instanceOf(Literal.class)); assertThat(((Literal) limit.limit()).value(), equalTo(11)); @@ -515,7 +533,7 @@ public void testInlineStatsWithinFork() { // second subplan eval = as(subPlans.get(1), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", literalString("fork2")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", literalString("fork2")))); var aggregate = as(eval.child(), Aggregate.class); assertThat(aggregate.aggregates().size(), equalTo(1)); var alias = as(aggregate.aggregates().get(0), Alias.class); @@ -526,7 +544,7 @@ public void testInlineStatsWithinFork() { // third subplan eval = as(subPlans.get(2), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", literalString("fork3")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", literalString("fork3")))); var inlineStats = as(eval.child(), InlineStats.class); aggregate = as(inlineStats.child(), Aggregate.class); assertThat(aggregate.aggregates().size(), equalTo(1)); @@ -936,12 +954,15 @@ public void testInvalidCharacterInIndexPattern() { } private void clustersAndIndices(String command, String indexString1, String indexString2) { - assertEquals(unresolvedRelation(indexString1 + "," + indexString2), statement(command, indexString1 + ", " + indexString2)); - assertEquals( + assertEqualsIgnoringIds( + unresolvedRelation(indexString1 + "," + indexString2), + statement(command, indexString1 + ", " + indexString2) + ); + assertEqualsIgnoringIds( unresolvedRelation(indexString1 + "," + indexString2), statement(command, indexString1 + ", \"" + indexString2 + "\"") ); - assertEquals( + assertEqualsIgnoringIds( unresolvedRelation(indexString1 + ", " + indexString2), statement(command, "\"" + indexString1 + ", " + indexString2 + "\"") ); @@ -1008,7 +1029,7 @@ public void testIdentifierAsFieldName() { assertThat(((UnresolvedAttribute) comparison.left()).name(), equalTo(expectedIdentifiers[j])); assertThat(comparison.right(), instanceOf(Literal.class)); assertThat(((Literal) comparison.right()).value(), equalTo(123)); - assertThat(filter.child(), equalTo(PROCESSING_CMD_INPUT)); + assertThat(filter.child(), equalToIgnoringIds(PROCESSING_CMD_INPUT)); } } } @@ -1017,7 +1038,7 @@ public void testBooleanLiteralCondition() { LogicalPlan where = processingCommand("where true"); assertThat(where, instanceOf(Filter.class)); Filter w = (Filter) where; - assertThat(w.child(), equalTo(PROCESSING_CMD_INPUT)); + assertThat(w.child(), equalToIgnoringIds(PROCESSING_CMD_INPUT)); assertThat(w.condition(), equalTo(TRUE)); } @@ -1081,7 +1102,7 @@ public void testBasicSortCommand() { public void testSubquery() { assumeTrue("Requires EXPLAIN capability", EsqlCapabilities.Cap.EXPLAIN.isEnabled()); - assertEquals(new Explain(EMPTY, PROCESSING_CMD_INPUT), statement("explain ( row a = 1 )")); + assertEqualsIgnoringIds(new Explain(EMPTY, PROCESSING_CMD_INPUT), statement("explain ( row a = 1 )")); } public void testBlockComments() { @@ -1094,7 +1115,7 @@ public void testBlockComments() { do { String queryWithComment = query.substring(0, wsIndex) + "/*explain ( \nfrom bar ) */" + query.substring(wsIndex + 1); - assertEquals(expected, statement(queryWithComment)); + assertEqualsIgnoringIds(expected, statement(queryWithComment)); wsIndex = query.indexOf(' ', wsIndex + 1); } while (wsIndex >= 0); @@ -1110,7 +1131,7 @@ public void testSingleLineComments() { do { String queryWithComment = query.substring(0, wsIndex) + "//explain ( from bar ) \n" + query.substring(wsIndex + 1); - assertEquals(expected, statement(queryWithComment)); + assertEqualsIgnoringIds(expected, statement(queryWithComment)); wsIndex = query.indexOf(' ', wsIndex + 1); } while (wsIndex >= 0); @@ -1122,7 +1143,7 @@ public void testNewLines() { LogicalPlan reference = statement(queryFun.apply(delims[0])); for (int i = 1; i < delims.length; i++) { LogicalPlan candidate = statement(queryFun.apply(delims[i])); - assertThat(candidate, equalTo(reference)); + assertThat(candidate, equalToIgnoringIds(reference)); } } @@ -1239,7 +1260,7 @@ public void testDissectPattern() { Dissect dissect = (Dissect) cmd; assertEquals("%{foo}", dissect.parser().pattern()); assertEquals("", dissect.parser().appendSeparator()); - assertEquals(List.of(referenceAttribute("foo", KEYWORD)), dissect.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("foo", KEYWORD)), dissect.extractedFields()); for (String separatorName : List.of("append_separator", "APPEND_SEPARATOR", "AppEnd_SeparAtor")) { cmd = processingCommand("dissect a \"%{foo}\" " + separatorName + "=\",\""); @@ -1247,7 +1268,7 @@ public void testDissectPattern() { dissect = (Dissect) cmd; assertEquals("%{foo}", dissect.parser().pattern()); assertEquals(",", dissect.parser().appendSeparator()); - assertEquals(List.of(referenceAttribute("foo", KEYWORD)), dissect.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("foo", KEYWORD)), dissect.extractedFields()); } for (Tuple queryWithUnexpectedCmd : List.of( @@ -1272,7 +1293,7 @@ public void testGrokPattern() { assertEquals(Grok.class, cmd.getClass()); Grok grok = (Grok) cmd; assertEquals("%{WORD:foo}", grok.parser().pattern()); - assertEquals(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); expectThrows( ParsingException.class, @@ -1284,7 +1305,7 @@ public void testGrokPattern() { assertEquals(Grok.class, cmd.getClass()); grok = (Grok) cmd; assertEquals("%{WORD:foo} %{WORD:foo}", grok.parser().pattern()); - assertEquals(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); expectError( "row a = \"foo bar\" | GROK a \"%{NUMBER:foo} %{WORD:foo}\"", @@ -1324,7 +1345,7 @@ public void testLikeRLike() { } public void testEnrich() { - assertEquals( + assertEqualsIgnoringIds( new Enrich( EMPTY, PROCESSING_CMD_INPUT, @@ -1338,7 +1359,7 @@ public void testEnrich() { processingCommand("enrich countries") ); - assertEquals( + assertEqualsIgnoringIds( new Enrich( EMPTY, PROCESSING_CMD_INPUT, @@ -1353,7 +1374,7 @@ public void testEnrich() { ); Enrich.Mode mode = randomFrom(Enrich.Mode.values()); - assertEquals( + assertEqualsIgnoringIds( new Enrich( EMPTY, PROCESSING_CMD_INPUT, @@ -1404,7 +1425,7 @@ public void testMvExpand() { LogicalPlan cmd = processingCommand("mv_expand a"); assertEquals(MvExpand.class, cmd.getClass()); MvExpand expand = (MvExpand) cmd; - assertThat(expand.target(), equalTo(attribute("a"))); + assertThat(expand.target(), equalToIgnoringIds(attribute("a"))); } // see https://github.com/elastic/elasticsearch/issues/103331 @@ -1918,7 +1939,7 @@ public void testParamForIdentifier() { // TODO will be replaced by testDoubleParamsForIdentifier after providing an identifier with a single parameter marker is deprecated // field names can appear in eval/where/stats/sort/keep/drop/rename/dissect/grok/enrich/mvexpand // eval, where - assertEquals( + assertEqualsIgnoringIds( new Limit( EMPTY, new Literal(EMPTY, 1, INTEGER), @@ -1945,7 +1966,7 @@ public void testParamForIdentifier() { ) ); - assertEquals( + assertEqualsIgnoringIds( new Limit( EMPTY, new Literal(EMPTY, 1, INTEGER), @@ -1977,7 +1998,7 @@ public void testParamForIdentifier() { ); // stats, sort, mv_expand - assertEquals( + assertEqualsIgnoringIds( new MvExpand( EMPTY, new OrderBy( @@ -2011,7 +2032,7 @@ public void testParamForIdentifier() { ) ); - assertEquals( + assertEqualsIgnoringIds( new MvExpand( EMPTY, new OrderBy( @@ -2068,21 +2089,21 @@ public void testParamForIdentifier() { ); Limit limit = as(plan, Limit.class); Rename rename = as(limit.child(), Rename.class); - assertEquals(rename.renamings(), List.of(new Alias(EMPTY, "f.8", attribute("f7*.")))); + assertEqualsIgnoringIds(rename.renamings(), List.of(new Alias(EMPTY, "f.8", attribute("f7*.")))); Grok grok = as(rename.child(), Grok.class); - assertEquals(grok.input(), attribute("f.6.")); + assertEqualsIgnoringIds(grok.input(), attribute("f.6.")); assertEquals("%{WORD:foo}", grok.parser().pattern()); - assertEquals(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); Dissect dissect = as(grok.child(), Dissect.class); - assertEquals(dissect.input(), attribute("f.5*")); + assertEqualsIgnoringIds(dissect.input(), attribute("f.5*")); assertEquals("%{bar}", dissect.parser().pattern()); assertEquals("", dissect.parser().appendSeparator()); - assertEquals(List.of(referenceAttribute("bar", KEYWORD)), dissect.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("bar", KEYWORD)), dissect.extractedFields()); Drop drop = as(dissect.child(), Drop.class); List removals = drop.removals(); - assertEquals(removals, List.of(attribute("f3."), attribute("f4.*"))); + assertEqualsIgnoringIds(removals, List.of(attribute("f3."), attribute("f4.*"))); Keep keep = as(drop.child(), Keep.class); - assertEquals(keep.projections(), List.of(attribute("f.1.*"), attribute("f.2"))); + assertEqualsIgnoringIds(keep.projections(), List.of(attribute("f.1.*"), attribute("f.2"))); plan = statement( """ @@ -2109,24 +2130,24 @@ public void testParamForIdentifier() { ); limit = as(plan, Limit.class); rename = as(limit.child(), Rename.class); - assertEquals(rename.renamings(), List.of(new Alias(EMPTY, "f11*..f.12", attribute("f.9*.f.10.")))); + assertEqualsIgnoringIds(rename.renamings(), List.of(new Alias(EMPTY, "f11*..f.12", attribute("f.9*.f.10.")))); grok = as(rename.child(), Grok.class); - assertEquals(grok.input(), attribute("f7*..f.8")); + assertEqualsIgnoringIds(grok.input(), attribute("f7*..f.8")); assertEquals("%{WORD:foo}", grok.parser().pattern()); - assertEquals(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); dissect = as(grok.child(), Dissect.class); - assertEquals(dissect.input(), attribute("f.5*.f.6.")); + assertEqualsIgnoringIds(dissect.input(), attribute("f.5*.f.6.")); assertEquals("%{bar}", dissect.parser().pattern()); assertEquals("", dissect.parser().appendSeparator()); - assertEquals(List.of(referenceAttribute("bar", KEYWORD)), dissect.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("bar", KEYWORD)), dissect.extractedFields()); drop = as(dissect.child(), Drop.class); removals = drop.removals(); - assertEquals(removals, List.of(attribute("f3..f4.*"))); + assertEqualsIgnoringIds(removals, List.of(attribute("f3..f4.*"))); keep = as(drop.child(), Keep.class); - assertEquals(keep.projections(), List.of(attribute("f.1.*.f.2"))); + assertEqualsIgnoringIds(keep.projections(), List.of(attribute("f.1.*.f.2"))); // enrich - assertEquals( + assertEqualsIgnoringIds( new Enrich( EMPTY, relation("idx1"), @@ -2143,7 +2164,7 @@ public void testParamForIdentifier() { ) ); - assertEquals( + assertEqualsIgnoringIds( new Enrich( EMPTY, relation("idx1"), @@ -2203,7 +2224,7 @@ public void testParamForIdentifierPattern() { assertEquals(up.name(), "f.2*"); assertEquals(up.pattern(), "f.2*"); UnresolvedRelation ur = as(keep.child(), UnresolvedRelation.class); - assertEquals(ur, relation("test")); + assertEqualsIgnoringIds(ur, relation("test")); plan = statement( "from test | keep ?f1.?f2 | drop ?f3.?f4", @@ -2229,7 +2250,7 @@ public void testParamForIdentifierPattern() { assertEquals(up.name(), "f*1..f.2*"); assertEquals(up.pattern(), "f*1..f.2*"); ur = as(keep.child(), UnresolvedRelation.class); - assertEquals(ur, relation("test")); + assertEqualsIgnoringIds(ur, relation("test")); // mixed names and patterns plan = statement( @@ -2256,7 +2277,7 @@ public void testParamForIdentifierPattern() { assertEquals("f*1..f.2**", up.name()); assertEquals("f*1..`f.2*`*", up.pattern()); ur = as(keep.child(), UnresolvedRelation.class); - assertEquals(ur, relation("test")); + assertEqualsIgnoringIds(ur, relation("test")); } public void testParamInInvalidPosition() { @@ -2345,7 +2366,7 @@ public void testFieldContainingDotsAndNumbers() { LogicalPlan where = processingCommand("where `a.b.1m.4321`"); assertThat(where, instanceOf(Filter.class)); Filter w = (Filter) where; - assertThat(w.child(), equalTo(PROCESSING_CMD_INPUT)); + assertThat(w.child(), equalToIgnoringIds(PROCESSING_CMD_INPUT)); assertThat(Expressions.name(w.condition()), equalTo("a.b.1m.4321")); } @@ -2353,7 +2374,7 @@ public void testFieldQualifiedName() { LogicalPlan where = processingCommand("where a.b.`1m`.`4321`"); assertThat(where, instanceOf(Filter.class)); Filter w = (Filter) where; - assertThat(w.child(), equalTo(PROCESSING_CMD_INPUT)); + assertThat(w.child(), equalToIgnoringIds(PROCESSING_CMD_INPUT)); assertThat(Expressions.name(w.condition()), equalTo("a.b.1m.4321")); } @@ -2704,7 +2725,7 @@ public void testFunctionNamedParameterInMap() { expectedMap3.put("option3", List.of(1, 2, 3)); expectedMap3.put("option4", List.of(true, false)); - assertEquals( + assertEqualsIgnoringIds( new Filter( EMPTY, new Eval( @@ -2732,7 +2753,7 @@ public void testFunctionNamedParameterInMap() { ); // In stats, by and sort as function named parameters - assertEquals( + assertEqualsIgnoringIds( new OrderBy( EMPTY, new Aggregate( @@ -2774,16 +2795,16 @@ by fn2(f3, {"option1":["string1","string2"],"option2":[1,2,3],"option3":2.0,"opt | grok fn2(f3, {"option1":["string1","string2"],"option2":[1,2,3],"option3":2.0,"option4":true}) "%{WORD:foo}" """); Grok grok = as(plan, Grok.class); - assertEquals(function("fn2", List.of(attribute("f3"), mapExpression(expectedMap2))), grok.input()); + assertEqualsIgnoringIds(function("fn2", List.of(attribute("f3"), mapExpression(expectedMap2))), grok.input()); assertEquals("%{WORD:foo}", grok.parser().pattern()); - assertEquals(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); Dissect dissect = as(grok.child(), Dissect.class); - assertEquals(function("fn1", List.of(attribute("f1"), attribute("f2"), mapExpression(expectedMap1))), dissect.input()); + assertEqualsIgnoringIds(function("fn1", List.of(attribute("f1"), attribute("f2"), mapExpression(expectedMap1))), dissect.input()); assertEquals("%{bar}", dissect.parser().pattern()); assertEquals("", dissect.parser().appendSeparator()); - assertEquals(List.of(referenceAttribute("bar", KEYWORD)), dissect.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("bar", KEYWORD)), dissect.extractedFields()); UnresolvedRelation ur = as(dissect.child(), UnresolvedRelation.class); - assertEquals(ur, relation("test")); + assertEqualsIgnoringIds(ur, relation("test")); } public void testFunctionNamedParameterInMapWithNamedParameters() { @@ -2803,7 +2824,7 @@ public void testFunctionNamedParameterInMapWithNamedParameters() { expectedMap3.put("option2", 2.0); expectedMap3.put("option3", List.of(1, 2, 3)); expectedMap3.put("option4", List.of(true, false)); - assertEquals( + assertEqualsIgnoringIds( new Filter( EMPTY, new Eval( @@ -2844,7 +2865,7 @@ public void testFunctionNamedParameterInMapWithNamedParameters() { ) ); - assertEquals( + assertEqualsIgnoringIds( new OrderBy( EMPTY, new Aggregate( @@ -2917,16 +2938,16 @@ public void testFunctionNamedParameterInMapWithNamedParameters() { ) ); Grok grok = as(plan, Grok.class); - assertEquals(function("fn2", List.of(attribute("f3"), mapExpression(expectedMap2))), grok.input()); + assertEqualsIgnoringIds(function("fn2", List.of(attribute("f3"), mapExpression(expectedMap2))), grok.input()); assertEquals("%{WORD:foo}", grok.parser().pattern()); - assertEquals(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); Dissect dissect = as(grok.child(), Dissect.class); - assertEquals(function("fn1", List.of(attribute("f1"), attribute("f2"), mapExpression(expectedMap1))), dissect.input()); + assertEqualsIgnoringIds(function("fn1", List.of(attribute("f1"), attribute("f2"), mapExpression(expectedMap1))), dissect.input()); assertEquals("%{bar}", dissect.parser().pattern()); assertEquals("", dissect.parser().appendSeparator()); - assertEquals(List.of(referenceAttribute("bar", KEYWORD)), dissect.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("bar", KEYWORD)), dissect.extractedFields()); UnresolvedRelation ur = as(dissect.child(), UnresolvedRelation.class); - assertEquals(ur, relation("test")); + assertEqualsIgnoringIds(ur, relation("test")); } public void testFunctionNamedParameterWithCaseSensitiveKeys() { @@ -2939,7 +2960,7 @@ public void testFunctionNamedParameterWithCaseSensitiveKeys() { expectedMap2.put("Option", List.of(1, 2, 3)); expectedMap2.put("oPtion", 2.0); - assertEquals( + assertEqualsIgnoringIds( new Filter( EMPTY, new Eval( @@ -3694,7 +3715,7 @@ public void testValidFork() { // first subplan var eval = as(subPlans.get(0), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", literalString("fork1")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", literalString("fork1")))); var limit = as(eval.child(), Limit.class); assertThat(limit.limit(), instanceOf(Literal.class)); assertThat(((Literal) limit.limit()).value(), equalTo(11)); @@ -3706,7 +3727,7 @@ public void testValidFork() { // second subplan eval = as(subPlans.get(1), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", literalString("fork2")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", literalString("fork2")))); var orderBy = as(eval.child(), OrderBy.class); assertThat(orderBy.order().size(), equalTo(1)); Order order = orderBy.order().get(0); @@ -3720,7 +3741,7 @@ public void testValidFork() { // third subplan eval = as(subPlans.get(2), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", literalString("fork3")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", literalString("fork3")))); filter = as(eval.child(), Filter.class); match = (MatchOperator) filter.condition(); matchField = (UnresolvedAttribute) match.field(); @@ -3729,7 +3750,7 @@ public void testValidFork() { // fourth subplan eval = as(subPlans.get(3), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", literalString("fork4")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", literalString("fork4")))); orderBy = as(eval.child(), OrderBy.class); assertThat(orderBy.order().size(), equalTo(1)); order = orderBy.order().get(0); @@ -3738,16 +3759,16 @@ public void testValidFork() { // fifth subplan eval = as(subPlans.get(4), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", literalString("fork5")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", literalString("fork5")))); limit = as(eval.child(), Limit.class); assertThat(limit.limit(), instanceOf(Literal.class)); assertThat(((Literal) limit.limit()).value(), equalTo(5)); // sixth subplan eval = as(subPlans.get(5), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("_fork", literalString("fork6")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("_fork", literalString("fork6")))); eval = as(eval.child(), Eval.class); - assertThat(as(eval.fields().get(0), Alias.class), equalTo(alias("xyz", literalString("abc")))); + assertThat(as(eval.fields().get(0), Alias.class), equalToIgnoringIds(alias("xyz", literalString("abc")))); Aggregate aggregate = as(eval.child(), Aggregate.class); assertThat(aggregate.aggregates().size(), equalTo(2)); @@ -3932,9 +3953,9 @@ public void testRerankDefaultInferenceIdAndScoreAttribute() { var rerank = as(plan, Rerank.class); assertThat(rerank.inferenceId(), equalTo(literalString(".rerank-v1-elasticsearch"))); - assertThat(rerank.scoreAttribute(), equalTo(attribute("_score"))); + assertThat(rerank.scoreAttribute(), equalToIgnoringIds(attribute("_score"))); assertThat(rerank.queryText(), equalTo(literalString("query text"))); - assertThat(rerank.rerankFields(), equalTo(List.of(alias("title", attribute("title"))))); + assertThat(rerank.rerankFields(), equalToIgnoringIds(List.of(alias("title", attribute("title"))))); } public void testRerankEmptyOptions() { @@ -3942,9 +3963,9 @@ public void testRerankEmptyOptions() { var rerank = as(plan, Rerank.class); assertThat(rerank.inferenceId(), equalTo(literalString(".rerank-v1-elasticsearch"))); - assertThat(rerank.scoreAttribute(), equalTo(attribute("_score"))); + assertThat(rerank.scoreAttribute(), equalToIgnoringIds(attribute("_score"))); assertThat(rerank.queryText(), equalTo(literalString("query text"))); - assertThat(rerank.rerankFields(), equalTo(List.of(alias("title", attribute("title"))))); + assertThat(rerank.rerankFields(), equalToIgnoringIds(List.of(alias("title", attribute("title"))))); } public void testRerankInferenceId() { @@ -3953,8 +3974,8 @@ public void testRerankInferenceId() { assertThat(rerank.inferenceId(), equalTo(literalString("inferenceId"))); assertThat(rerank.queryText(), equalTo(literalString("query text"))); - assertThat(rerank.rerankFields(), equalTo(List.of(alias("title", attribute("title"))))); - assertThat(rerank.scoreAttribute(), equalTo(attribute("_score"))); + assertThat(rerank.rerankFields(), equalToIgnoringIds(List.of(alias("title", attribute("title"))))); + assertThat(rerank.scoreAttribute(), equalToIgnoringIds(attribute("_score"))); } public void testRerankScoreAttribute() { @@ -3962,9 +3983,9 @@ public void testRerankScoreAttribute() { var rerank = as(plan, Rerank.class); assertThat(rerank.inferenceId(), equalTo(literalString(".rerank-v1-elasticsearch"))); - assertThat(rerank.scoreAttribute(), equalTo(attribute("rerank_score"))); + assertThat(rerank.scoreAttribute(), equalToIgnoringIds(attribute("rerank_score"))); assertThat(rerank.queryText(), equalTo(literalString("query text"))); - assertThat(rerank.rerankFields(), equalTo(List.of(alias("title", attribute("title"))))); + assertThat(rerank.rerankFields(), equalToIgnoringIds(List.of(alias("title", attribute("title"))))); } public void testRerankInferenceIdAnddScoreAttribute() { @@ -3972,9 +3993,9 @@ public void testRerankInferenceIdAnddScoreAttribute() { var rerank = as(plan, Rerank.class); assertThat(rerank.inferenceId(), equalTo(literalString("inferenceId"))); - assertThat(rerank.scoreAttribute(), equalTo(attribute("rerank_score"))); + assertThat(rerank.scoreAttribute(), equalToIgnoringIds(attribute("rerank_score"))); assertThat(rerank.queryText(), equalTo(literalString("query text"))); - assertThat(rerank.rerankFields(), equalTo(List.of(alias("title", attribute("title"))))); + assertThat(rerank.rerankFields(), equalToIgnoringIds(List.of(alias("title", attribute("title"))))); } public void testRerankSingleField() { @@ -3983,8 +4004,8 @@ public void testRerankSingleField() { assertThat(rerank.queryText(), equalTo(literalString("query text"))); assertThat(rerank.inferenceId(), equalTo(literalString("inferenceID"))); - assertThat(rerank.rerankFields(), equalTo(List.of(alias("title", attribute("title"))))); - assertThat(rerank.scoreAttribute(), equalTo(attribute("_score"))); + assertThat(rerank.rerankFields(), equalToIgnoringIds(List.of(alias("title", attribute("title"))))); + assertThat(rerank.scoreAttribute(), equalToIgnoringIds(attribute("_score"))); } public void testRerankMultipleFields() { @@ -3997,7 +4018,7 @@ public void testRerankMultipleFields() { assertThat(rerank.inferenceId(), equalTo(literalString("inferenceID"))); assertThat( rerank.rerankFields(), - equalTo( + equalToIgnoringIds( List.of( alias("title", attribute("title")), alias("description", attribute("description")), @@ -4005,7 +4026,7 @@ public void testRerankMultipleFields() { ) ) ); - assertThat(rerank.scoreAttribute(), equalTo(attribute("_score"))); + assertThat(rerank.scoreAttribute(), equalToIgnoringIds(attribute("_score"))); } public void testRerankComputedFields() { @@ -4018,14 +4039,14 @@ public void testRerankComputedFields() { assertThat(rerank.inferenceId(), equalTo(literalString("inferenceID"))); assertThat( rerank.rerankFields(), - equalTo( + equalToIgnoringIds( List.of( alias("title", attribute("title")), alias("short_description", function("SUBSTRING", List.of(attribute("description"), integer(0), integer(100)))) ) ) ); - assertThat(rerank.scoreAttribute(), equalTo(attribute("_score"))); + assertThat(rerank.scoreAttribute(), equalToIgnoringIds(attribute("_score"))); } public void testRerankComputedFieldsWithoutName() { @@ -4045,8 +4066,8 @@ public void testRerankWithPositionalParameters() { assertThat(rerank.queryText(), equalTo(literalString("query text"))); assertThat(rerank.inferenceId(), equalTo(literalString("reranker"))); - assertThat(rerank.rerankFields(), equalTo(List.of(alias("title", attribute("title"))))); - assertThat(rerank.scoreAttribute(), equalTo(attribute("rerank_score"))); + assertThat(rerank.rerankFields(), equalToIgnoringIds(List.of(alias("title", attribute("title"))))); + assertThat(rerank.scoreAttribute(), equalToIgnoringIds(attribute("rerank_score"))); } public void testRerankWithNamedParameters() { @@ -4061,8 +4082,8 @@ public void testRerankWithNamedParameters() { assertThat(rerank.queryText(), equalTo(literalString("query text"))); assertThat(rerank.inferenceId(), equalTo(literalString("reranker"))); - assertThat(rerank.rerankFields(), equalTo(List.of(alias("title", attribute("title"))))); - assertThat(rerank.scoreAttribute(), equalTo(attribute("rerank_score"))); + assertThat(rerank.rerankFields(), equalToIgnoringIds(List.of(alias("title", attribute("title"))))); + assertThat(rerank.scoreAttribute(), equalToIgnoringIds(attribute("rerank_score"))); } public void testInvalidRerank() { @@ -4106,9 +4127,9 @@ public void testCompletionUsingFieldAsPrompt() { Completion.class ); - assertThat(plan.prompt(), equalTo(attribute("prompt_field"))); + assertThat(plan.prompt(), equalToIgnoringIds(attribute("prompt_field"))); assertThat(plan.inferenceId(), equalTo(literalString("inferenceID"))); - assertThat(plan.targetField(), equalTo(attribute("targetField"))); + assertThat(plan.targetField(), equalToIgnoringIds(attribute("targetField"))); } public void testCompletionUsingFunctionAsPrompt() { @@ -4117,17 +4138,17 @@ public void testCompletionUsingFunctionAsPrompt() { Completion.class ); - assertThat(plan.prompt(), equalTo(function("CONCAT", List.of(attribute("fieldA"), attribute("fieldB"))))); + assertThat(plan.prompt(), equalToIgnoringIds(function("CONCAT", List.of(attribute("fieldA"), attribute("fieldB"))))); assertThat(plan.inferenceId(), equalTo(literalString("inferenceID"))); - assertThat(plan.targetField(), equalTo(attribute("targetField"))); + assertThat(plan.targetField(), equalToIgnoringIds(attribute("targetField"))); } public void testCompletionDefaultFieldName() { var plan = as(processingCommand("COMPLETION prompt_field WITH{ \"inference_id\" : \"inferenceID\" }"), Completion.class); - assertThat(plan.prompt(), equalTo(attribute("prompt_field"))); + assertThat(plan.prompt(), equalToIgnoringIds(attribute("prompt_field"))); assertThat(plan.inferenceId(), equalTo(literalString("inferenceID"))); - assertThat(plan.targetField(), equalTo(attribute("completion"))); + assertThat(plan.targetField(), equalToIgnoringIds(attribute("completion"))); } public void testCompletionWithPositionalParameters() { @@ -4137,9 +4158,9 @@ public void testCompletionWithPositionalParameters() { Completion.class ); - assertThat(plan.prompt(), equalTo(attribute("prompt_field"))); + assertThat(plan.prompt(), equalToIgnoringIds(attribute("prompt_field"))); assertThat(plan.inferenceId(), equalTo(literalString("inferenceId"))); - assertThat(plan.targetField(), equalTo(attribute("completion"))); + assertThat(plan.targetField(), equalToIgnoringIds(attribute("completion"))); } public void testCompletionWithNamedParameters() { @@ -4149,9 +4170,9 @@ public void testCompletionWithNamedParameters() { Completion.class ); - assertThat(plan.prompt(), equalTo(attribute("prompt_field"))); + assertThat(plan.prompt(), equalToIgnoringIds(attribute("prompt_field"))); assertThat(plan.inferenceId(), equalTo(literalString("myInference"))); - assertThat(plan.targetField(), equalTo(attribute("completion"))); + assertThat(plan.targetField(), equalToIgnoringIds(attribute("completion"))); } public void testInvalidCompletion() { @@ -4345,7 +4366,7 @@ public void testDoubleParamsForIdentifier() { | eval {} = {}({}) | where {} == {} | limit 1""", params.get(0), params.get(1), params.get(2), params.get(3), params.get(4)); - assertEquals( + assertEqualsIgnoringIds( new Limit( EMPTY, new Literal(EMPTY, 1, INTEGER), @@ -4394,7 +4415,7 @@ public void testDoubleParamsForIdentifier() { params.get(6), params.get(7) ); - assertEquals( + assertEqualsIgnoringIds( new Limit( EMPTY, new Literal(EMPTY, 1, INTEGER), @@ -4440,7 +4461,7 @@ public void testDoubleParamsForIdentifier() { | stats y = {}({}) by {} | sort {} | mv_expand {}""", params.get(0), params.get(1), params.get(2), params.get(3), params.get(4)); - assertEquals( + assertEqualsIgnoringIds( new MvExpand( EMPTY, new OrderBy( @@ -4496,7 +4517,7 @@ public void testDoubleParamsForIdentifier() { params.get(7), params.get(8) ); - assertEquals( + assertEqualsIgnoringIds( new MvExpand( EMPTY, new OrderBy( @@ -4583,23 +4604,23 @@ public void testDoubleParamsForIdentifier() { assertEquals(ur.indexPattern().indexPattern(), "idx"); JoinTypes.UsingJoinType joinType = as(join.config().type(), JoinTypes.UsingJoinType.class); assertEquals(joinType.coreJoin().joinName(), "LEFT OUTER"); - assertEquals(joinType.columns(), List.of(attribute("f9"))); + assertEqualsIgnoringIds(joinType.columns(), List.of(attribute("f9"))); Rename rename = as(join.left(), Rename.class); - assertEquals(rename.renamings(), List.of(new Alias(EMPTY, "f.8", attribute("f7*.")))); + assertEqualsIgnoringIds(rename.renamings(), List.of(new Alias(EMPTY, "f.8", attribute("f7*.")))); Grok grok = as(rename.child(), Grok.class); - assertEquals(grok.input(), attribute("f.6.")); + assertEqualsIgnoringIds(grok.input(), attribute("f.6.")); assertEquals("%{WORD:foo}", grok.parser().pattern()); - assertEquals(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); Dissect dissect = as(grok.child(), Dissect.class); - assertEquals(dissect.input(), attribute("f.5*")); + assertEqualsIgnoringIds(dissect.input(), attribute("f.5*")); assertEquals("%{bar}", dissect.parser().pattern()); assertEquals("", dissect.parser().appendSeparator()); - assertEquals(List.of(referenceAttribute("bar", KEYWORD)), dissect.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("bar", KEYWORD)), dissect.extractedFields()); Drop drop = as(dissect.child(), Drop.class); List removals = drop.removals(); - assertEquals(removals, List.of(attribute("f3."), attribute("f4.*"))); + assertEqualsIgnoringIds(removals, List.of(attribute("f3."), attribute("f4.*"))); Keep keep = as(drop.child(), Keep.class); - assertEquals(keep.projections(), List.of(attribute("f.1.*"), attribute("f.2"))); + assertEqualsIgnoringIds(keep.projections(), List.of(attribute("f.1.*"), attribute("f.2"))); } namedDoubleParams = List.of( @@ -4693,23 +4714,23 @@ public void testDoubleParamsForIdentifier() { assertEquals(ur.indexPattern().indexPattern(), "idx"); JoinTypes.UsingJoinType joinType = as(join.config().type(), JoinTypes.UsingJoinType.class); assertEquals(joinType.coreJoin().joinName(), "LEFT OUTER"); - assertEquals(joinType.columns(), List.of(attribute("f13.f14"))); + assertEqualsIgnoringIds(joinType.columns(), List.of(attribute("f13.f14"))); Rename rename = as(join.left(), Rename.class); - assertEquals(rename.renamings(), List.of(new Alias(EMPTY, "f11*..f.12", attribute("f.9*.f.10.")))); + assertEqualsIgnoringIds(rename.renamings(), List.of(new Alias(EMPTY, "f11*..f.12", attribute("f.9*.f.10.")))); Grok grok = as(rename.child(), Grok.class); - assertEquals(grok.input(), attribute("f7*..f.8")); + assertEqualsIgnoringIds(grok.input(), attribute("f7*..f.8")); assertEquals("%{WORD:foo}", grok.parser().pattern()); - assertEquals(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("foo", KEYWORD)), grok.extractedFields()); Dissect dissect = as(grok.child(), Dissect.class); - assertEquals(dissect.input(), attribute("f.5*.f.6.")); + assertEqualsIgnoringIds(dissect.input(), attribute("f.5*.f.6.")); assertEquals("%{bar}", dissect.parser().pattern()); assertEquals("", dissect.parser().appendSeparator()); - assertEquals(List.of(referenceAttribute("bar", KEYWORD)), dissect.extractedFields()); + assertEqualsIgnoringIds(List.of(referenceAttribute("bar", KEYWORD)), dissect.extractedFields()); Drop drop = as(dissect.child(), Drop.class); List removals = drop.removals(); - assertEquals(removals, List.of(attribute("f3..f4.*"))); + assertEqualsIgnoringIds(removals, List.of(attribute("f3..f4.*"))); Keep keep = as(drop.child(), Keep.class); - assertEquals(keep.projections(), List.of(attribute("f.1.*.f.2"))); + assertEqualsIgnoringIds(keep.projections(), List.of(attribute("f.1.*.f.2"))); } // enrich, lookup join @@ -4728,7 +4749,7 @@ public void testDoubleParamsForIdentifier() { params.get(1), params.get(2) ); - assertEquals( + assertEqualsIgnoringIds( new Enrich( EMPTY, relation("idx1"), @@ -4799,7 +4820,7 @@ public void testDoubleParamsForIdentifier() { params.get(4), params.get(5) ); - assertEquals( + assertEqualsIgnoringIds( new Enrich( EMPTY, relation("idx1"), @@ -4851,7 +4872,7 @@ public void testMixedSingleDoubleParams() { | eval {} = {}({}) | where {} == {} | limit 1""", params.get(0), params.get(1), params.get(2), params.get(3), params.get(4)); - assertEquals( + assertEqualsIgnoringIds( new Limit( EMPTY, new Literal(EMPTY, 1, INTEGER), @@ -4894,7 +4915,7 @@ public void testMixedSingleDoubleParams() { | stats y = {}({}) by {} | sort {} | mv_expand {}""", params.get(0), params.get(1), params.get(2), params.get(3), params.get(4)); - assertEquals( + assertEqualsIgnoringIds( new MvExpand( EMPTY, new OrderBy( @@ -4944,7 +4965,7 @@ public void testMixedSingleDoubleParams() { assertEquals(ur.indexPattern().indexPattern(), "idx"); JoinTypes.UsingJoinType joinType = as(join.config().type(), JoinTypes.UsingJoinType.class); assertEquals(joinType.coreJoin().joinName(), "LEFT OUTER"); - assertEquals(joinType.columns(), List.of(attribute("f5"))); + assertEqualsIgnoringIds(joinType.columns(), List.of(attribute("f5"))); Drop drop = as(join.left(), Drop.class); List removals = drop.removals(); assertEquals(removals.size(), 2); @@ -4961,7 +4982,7 @@ public void testMixedSingleDoubleParams() { assertEquals(up.name(), "f.2*"); assertEquals(up.pattern(), "f.2*"); ur = as(keep.child(), UnresolvedRelation.class); - assertEquals(ur, relation("test")); + assertEqualsIgnoringIds(ur, relation("test")); // test random single and double params // commands in group1 take both constants(?) and identifiers(??) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/AbstractNodeSerializationTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/AbstractNodeSerializationTests.java index 998b895a4e005..d986b3c41b1ae 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/AbstractNodeSerializationTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/AbstractNodeSerializationTests.java @@ -9,6 +9,7 @@ import org.elasticsearch.TransportVersion; import org.elasticsearch.test.AbstractWireTestCase; +import org.elasticsearch.xpack.esql.SerializationTestUtils; import org.elasticsearch.xpack.esql.core.expression.Attribute; import org.elasticsearch.xpack.esql.core.tree.Node; import org.elasticsearch.xpack.esql.core.tree.Source; @@ -27,8 +28,9 @@ import static org.hamcrest.Matchers.sameInstance; /** - * Superclass for serialization tests for all {@link Node} subclasses - * @param + * Superclass for serialization tests for all {@link Node} subclasses. + * Useful because it provides a {@link #configuration()} method to access a random + * {@link Configuration} and respects {@link org.elasticsearch.xpack.esql.core.expression.NameId}s. */ public abstract class AbstractNodeSerializationTests> extends AbstractWireTestCase { /** @@ -57,7 +59,12 @@ protected T copyInstance(T instance, TransportVersion version) throws IOExceptio getNamedWriteableRegistry(), (out, v) -> new PlanStreamOutput(out, configuration()).writeNamedWriteable(v), in -> { - PlanStreamInput pin = new PlanStreamInput(in, in.namedWriteableRegistry(), configuration()); + PlanStreamInput pin = new PlanStreamInput( + in, + in.namedWriteableRegistry(), + configuration(), + new SerializationTestUtils.TestNameIdMapper() + ); @SuppressWarnings("unchecked") T deser = (T) pin.readNamedWriteable(categoryClass()); if (alwaysEmptySource()) { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/physical/ExchangeSinkExecSerializationTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/physical/ExchangeSinkExecSerializationTests.java index f4effe507ab64..95c8cdaf186ea 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/physical/ExchangeSinkExecSerializationTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/physical/ExchangeSinkExecSerializationTests.java @@ -10,6 +10,7 @@ import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.index.IndexMode; +import org.elasticsearch.xpack.esql.SerializationTestUtils; import org.elasticsearch.xpack.esql.analysis.Analyzer; import org.elasticsearch.xpack.esql.core.expression.Attribute; import org.elasticsearch.xpack.esql.core.expression.Literal; @@ -212,7 +213,14 @@ private void testSerializePlanWithIndex(EsIndex index, ByteSizeValue expected, b try (BytesStreamOutput out = new BytesStreamOutput(); PlanStreamOutput pso = new PlanStreamOutput(out, configuration())) { pso.writeNamedWriteable(exchangeSinkExec); assertThat(ByteSizeValue.ofBytes(out.bytes().length()), byteSizeEquals(expected)); - try (PlanStreamInput psi = new PlanStreamInput(out.bytes().streamInput(), getNamedWriteableRegistry(), configuration())) { + try ( + PlanStreamInput psi = new PlanStreamInput( + out.bytes().streamInput(), + getNamedWriteableRegistry(), + configuration(), + new SerializationTestUtils.TestNameIdMapper() + ) + ) { assertThat(psi.readNamedWriteable(PhysicalPlan.class), equalTo(exchangeSinkExec)); } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plugin/ClusterRequestTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plugin/ClusterRequestTests.java index b25fba6f1dc48..caddfe4bf94b4 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plugin/ClusterRequestTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plugin/ClusterRequestTests.java @@ -18,6 +18,7 @@ import org.elasticsearch.test.AbstractWireSerializingTestCase; import org.elasticsearch.xpack.esql.ConfigurationTestUtils; import org.elasticsearch.xpack.esql.EsqlTestUtils; +import org.elasticsearch.xpack.esql.SerializationTestUtils; import org.elasticsearch.xpack.esql.analysis.Analyzer; import org.elasticsearch.xpack.esql.analysis.AnalyzerContext; import org.elasticsearch.xpack.esql.core.type.EsField; @@ -49,7 +50,7 @@ public class ClusterRequestTests extends AbstractWireSerializingTestCase instanceReader() { - return ClusterComputeRequest::new; + return (in) -> new ClusterComputeRequest(in, new SerializationTestUtils.TestNameIdMapper()); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequestSerializationTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequestSerializationTests.java index 48a640d15514b..d1dac410aac33 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequestSerializationTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequestSerializationTests.java @@ -19,6 +19,7 @@ import org.elasticsearch.search.SearchModule; import org.elasticsearch.search.internal.AliasFilter; import org.elasticsearch.test.AbstractWireSerializingTestCase; +import org.elasticsearch.xpack.esql.SerializationTestUtils; import org.elasticsearch.xpack.esql.analysis.Analyzer; import org.elasticsearch.xpack.esql.core.expression.FoldContext; import org.elasticsearch.xpack.esql.core.type.EsField; @@ -51,10 +52,9 @@ import static org.elasticsearch.xpack.esql.EsqlTestUtils.withDefaultLimitWarning; public class DataNodeRequestSerializationTests extends AbstractWireSerializingTestCase { - @Override protected Writeable.Reader instanceReader() { - return DataNodeRequest::new; + return in -> new DataNodeRequest(in, new SerializationTestUtils.TestNameIdMapper()); } @Override diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/tree/EsqlNodeSubclassTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/tree/EsqlNodeSubclassTests.java index 2f848d07f657f..d3ce61ef372bd 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/tree/EsqlNodeSubclassTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/tree/EsqlNodeSubclassTests.java @@ -26,7 +26,6 @@ import org.elasticsearch.xpack.esql.core.expression.FieldAttribute; import org.elasticsearch.xpack.esql.core.expression.Literal; import org.elasticsearch.xpack.esql.core.expression.UnresolvedAttribute; -import org.elasticsearch.xpack.esql.core.expression.UnresolvedAttributeTests; import org.elasticsearch.xpack.esql.core.expression.UnresolvedNamedExpression; import org.elasticsearch.xpack.esql.core.expression.function.Function; import org.elasticsearch.xpack.esql.core.tree.AbstractNodeTestCase; @@ -39,6 +38,7 @@ import org.elasticsearch.xpack.esql.core.type.DataType; import org.elasticsearch.xpack.esql.core.type.EsField; import org.elasticsearch.xpack.esql.expression.Order; +import org.elasticsearch.xpack.esql.expression.UnresolvedAttributeTests; import org.elasticsearch.xpack.esql.expression.function.UnresolvedFunction; import org.elasticsearch.xpack.esql.expression.function.scalar.ip.CIDRMatch; import org.elasticsearch.xpack.esql.expression.function.scalar.math.Pow; diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/AbstractEsFieldTypeTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/AbstractEsFieldTypeTests.java index 44a83f9c964c2..d6321d9880de3 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/AbstractEsFieldTypeTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/AbstractEsFieldTypeTests.java @@ -14,16 +14,18 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.test.AbstractWireTestCase; import org.elasticsearch.xpack.esql.EsqlTestUtils; +import org.elasticsearch.xpack.esql.SerializationTestUtils; import org.elasticsearch.xpack.esql.core.type.EsField; import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput; import org.elasticsearch.xpack.esql.io.stream.PlanStreamOutput; +import org.elasticsearch.xpack.esql.session.Configuration; import java.io.IOException; import java.util.List; import java.util.Map; import java.util.TreeMap; -public abstract class AbstractEsFieldTypeTests extends AbstractWireTestCase { +public abstract class AbstractEsFieldTypeTests extends AbstractWireTestCase { public static EsField randomAnyEsField(int maxDepth) { return switch (between(0, 5)) { case 0 -> EsFieldTests.randomEsField(maxDepth); @@ -39,17 +41,15 @@ public static EsField randomAnyEsField(int maxDepth) { @Override protected abstract T createTestInstance(); - protected abstract T mutate(T instance); - @Override - protected EsField copyInstance(EsField instance, TransportVersion version) throws IOException { + protected T copyInstance(T instance, TransportVersion version) throws IOException { NamedWriteableRegistry namedWriteableRegistry = getNamedWriteableRegistry(); try (BytesStreamOutput output = new BytesStreamOutput(); var pso = new PlanStreamOutput(output, EsqlTestUtils.TEST_CFG)) { pso.setTransportVersion(version); instance.writeTo(pso); try ( StreamInput in = new NamedWriteableAwareStreamInput(output.bytes().streamInput(), namedWriteableRegistry); - var psi = new PlanStreamInput(in, in.namedWriteableRegistry(), EsqlTestUtils.TEST_CFG) + var psi = new PlanStreamInput(in, in.namedWriteableRegistry(), config(), new SerializationTestUtils.TestNameIdMapper()) ) { psi.setTransportVersion(version); return EsField.readFrom(psi); @@ -57,6 +57,10 @@ protected EsField copyInstance(EsField instance, TransportVersion version) throw } } + protected Configuration config() { + return EsqlTestUtils.TEST_CFG; + } + /** * Generate sub-properties. * @param maxDepth the maximum number of levels of properties to make @@ -77,13 +81,7 @@ static Map randomProperties(int maxDepth) { } @Override - @SuppressWarnings("unchecked") - protected final T mutateInstance(EsField instance) throws IOException { - return mutate((T) instance); - } - - @Override - protected final NamedWriteableRegistry getNamedWriteableRegistry() { + protected NamedWriteableRegistry getNamedWriteableRegistry() { return new NamedWriteableRegistry(List.of()); } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/DateEsFieldTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/DateEsFieldTests.java index a7cf5b499e7ee..ce528451e4830 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/DateEsFieldTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/DateEsFieldTests.java @@ -28,7 +28,7 @@ protected DateEsField createTestInstance() { } @Override - protected DateEsField mutate(DateEsField instance) { + protected DateEsField mutateInstance(DateEsField instance) { String name = instance.getName(); Map properties = instance.getProperties(); boolean aggregatable = instance.isAggregatable(); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/EsFieldTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/EsFieldTests.java index c29cbde8a7877..bac69261f24b5 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/EsFieldTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/EsFieldTests.java @@ -29,7 +29,7 @@ protected EsField createTestInstance() { } @Override - protected EsField mutate(EsField instance) { + protected EsField mutateInstance(EsField instance) { String name = instance.getName(); DataType esDataType = instance.getDataType(); Map properties = instance.getProperties(); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/InvalidMappedFieldTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/InvalidMappedFieldTests.java index c66088b0695d4..fc35e940a8c13 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/InvalidMappedFieldTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/InvalidMappedFieldTests.java @@ -26,7 +26,7 @@ protected InvalidMappedField createTestInstance() { } @Override - protected InvalidMappedField mutate(InvalidMappedField instance) { + protected InvalidMappedField mutateInstance(InvalidMappedField instance) { String name = instance.getName(); String errorMessage = instance.errorMessage(); Map properties = instance.getProperties(); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/KeywordEsFieldTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/KeywordEsFieldTests.java index 9441b2e896f10..c451ed5e12958 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/KeywordEsFieldTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/KeywordEsFieldTests.java @@ -31,7 +31,7 @@ protected KeywordEsField createTestInstance() { } @Override - protected KeywordEsField mutate(KeywordEsField instance) { + protected KeywordEsField mutateInstance(KeywordEsField instance) { String name = instance.getName(); Map properties = instance.getProperties(); boolean hasDocValues = instance.isAggregatable(); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/MultiTypeEsFieldTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/MultiTypeEsFieldTests.java index f64d2417aff1d..e4ed99dd0785a 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/MultiTypeEsFieldTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/MultiTypeEsFieldTests.java @@ -7,9 +7,7 @@ package org.elasticsearch.xpack.esql.type; -import org.elasticsearch.TransportVersion; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; -import org.elasticsearch.test.AbstractWireTestCase; import org.elasticsearch.xpack.esql.core.expression.Expression; import org.elasticsearch.xpack.esql.core.expression.FieldAttribute; import org.elasticsearch.xpack.esql.core.tree.Source; @@ -29,8 +27,6 @@ import org.elasticsearch.xpack.esql.expression.function.scalar.convert.ToLong; import org.elasticsearch.xpack.esql.expression.function.scalar.convert.ToString; import org.elasticsearch.xpack.esql.expression.function.scalar.convert.ToVersion; -import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput; -import org.elasticsearch.xpack.esql.io.stream.PlanStreamOutput; import org.elasticsearch.xpack.esql.session.Configuration; import org.junit.Before; @@ -45,18 +41,10 @@ /** * This test was originally based on the tests for sub-classes of EsField, like InvalidMappedFieldTests. - * However, it has a few important differences: - *
    - *
  • It is not in the esql.core module, but in the esql module, in order to have access to the sub-classes of AbstractConvertFunction, - * like ToString, which are important conversion Expressions used in the union-types feature.
    • - *
  • It extends AbstractNamedWriteableTestCase instead of AbstractEsFieldTypeTests, - * in order to wrap the StreamInput with a PlanStreamInput, since Expression is not yet fully supported in the new - * serialization approach (NamedWritable).
    • - *
- * These differences can be minimized once Expression is fully supported in the new serialization approach, and the esql and esql.core - * modules are merged, or at least the relevant classes are moved. + * However, it needs access to the sub-classes of AbstractConvertFunction, like ToString, which are important conversion Expressions + * used in the union-types feature. */ -public class MultiTypeEsFieldTests extends AbstractWireTestCase { +public class MultiTypeEsFieldTests extends AbstractEsFieldTypeTests { private Configuration config; @@ -65,6 +53,11 @@ public void initConfig() { config = randomConfiguration(); } + @Override + protected Configuration config() { + return config; + } + @Override protected MultiTypeEsField createTestInstance() { String name = randomAlphaOfLength(4); @@ -99,14 +92,6 @@ protected final NamedWriteableRegistry getNamedWriteableRegistry() { return new NamedWriteableRegistry(entries); } - @Override - protected final MultiTypeEsField copyInstance(MultiTypeEsField instance, TransportVersion version) throws IOException { - return copyInstance(instance, getNamedWriteableRegistry(), (out, v) -> v.writeTo(new PlanStreamOutput(out, config)), in -> { - PlanStreamInput pin = new PlanStreamInput(in, in.namedWriteableRegistry(), config); - return EsField.readFrom(pin); - }, version); - } - private static Map randomConvertExpressions(String name, boolean toString, DataType dataType) { Map indexToConvertExpressions = new HashMap<>(); if (toString) { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/TextEsFieldTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/TextEsFieldTests.java index cc2270d94ddfd..2aa4d5e70c5c6 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/TextEsFieldTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/TextEsFieldTests.java @@ -28,7 +28,7 @@ protected TextEsField createTestInstance() { } @Override - protected TextEsField mutate(TextEsField instance) { + protected TextEsField mutateInstance(TextEsField instance) { String name = instance.getName(); Map properties = instance.getProperties(); boolean hasDocValues = instance.isAggregatable(); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/UnsupportedEsFieldTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/UnsupportedEsFieldTests.java index 908cdb279613c..c9de76f9425f9 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/UnsupportedEsFieldTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/UnsupportedEsFieldTests.java @@ -32,7 +32,7 @@ protected UnsupportedEsField createTestInstance() { } @Override - protected UnsupportedEsField mutate(UnsupportedEsField instance) { + protected UnsupportedEsField mutateInstance(UnsupportedEsField instance) { String name = instance.getName(); List originalTypes = instance.getOriginalTypes(); String inherited = instance.getInherited(); From 966fa6b1f142077c9ee7d7a13814e1536f60f715 Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Wed, 22 Oct 2025 14:06:58 +0200 Subject: [PATCH 2/2] Fix id ignoring matcher for UsingJoinType --- .../xpack/esql/EsqlTestUtils.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java index 0fb0db111c4a8..e0695816db86c 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java @@ -105,6 +105,9 @@ import org.elasticsearch.xpack.esql.plan.logical.Explain; import org.elasticsearch.xpack.esql.plan.logical.Limit; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; +import org.elasticsearch.xpack.esql.plan.logical.join.JoinConfig; +import org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes; +import org.elasticsearch.xpack.esql.plan.logical.join.LookupJoin; import org.elasticsearch.xpack.esql.plan.logical.local.EmptyLocalSupplier; import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation; import org.elasticsearch.xpack.esql.plan.logical.local.LocalSupplier; @@ -1161,6 +1164,27 @@ private static LogicalPlan ignoreIdsInLogicalPlan(LogicalPlan plan) { return new Explain(explain.source(), ignoreIdsInLogicalPlan(explain.query())); } + // For LookupJoin with the UsingJoinType, the join keys are not part of the plan expressions/node info, + // so transformExpressionsDown won't reach them. We need to handle them specifically. + plan = plan.transformDown(LookupJoin.class, lookupJoin -> { + JoinConfig config = lookupJoin.config(); + if (config.type() instanceof JoinTypes.UsingJoinType usingJoinType) { + List newJoinKeys = usingJoinType.columns() + .stream() + .map(attr -> (Attribute) ignoreIdsInExpression(attr)) + .toList(); + JoinTypes.UsingJoinType newJoinType = new JoinTypes.UsingJoinType(usingJoinType.coreJoin(), newJoinKeys); + JoinConfig newJoinConfig = new JoinConfig( + newJoinType, + config.leftFields(), + config.rightFields(), + config.joinOnConditions() + ); + return new LookupJoin(lookupJoin.source(), lookupJoin.left(), lookupJoin.right(), newJoinConfig, lookupJoin.isRemote()); + } + return lookupJoin; + }); + return plan.transformExpressionsDown( NamedExpression.class, ne -> ne instanceof Alias alias ? alias.withId(DUMMY_ID) : ne instanceof Attribute attr ? attr.withId(DUMMY_ID) : ne