Fix for union-types for multiple columns with the same name

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

@@ -29,6 +29,10 @@
  * - nestedParent - if nested, what's the parent (which might not be the immediate one)
  */
 public class FieldAttribute extends TypedAttribute {
+    // TODO: This constant should not be used if possible; use .synthetic()
+    // https://github.com/elastic/elasticsearch/issues/105821
+    public static final String SYNTHETIC_ATTRIBUTE_NAME_PREFIX = "$$";
+
     static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(
         Attribute.class,
         "FieldAttribute",
@@ -72,7 +76,7 @@ public FieldAttribute(
         boolean synthetic
     ) {
         super(source, name, type, qualifier, nullability, id, synthetic);
-        this.path = parent != null ? parent.name() : StringUtils.EMPTY;
+        this.path = parent != null ? parent.fieldName() : StringUtils.EMPTY;
         this.parent = parent;
         this.field = field;
     }
@@ -131,6 +135,22 @@ public String path() {
         return path;
     }
 
+    /**
+     * The full name of the field in the index, including all parent fields. E.g. {@code parent.subfield.this_field}.
+     */
+    public String fieldName() {
+        // Before 8.15, the field name was the same as the attribute's name.
+        // On later versions, the attribute can be renamed when creating synthetic attributes.
+        // TODO: We should use synthetic() to check for that case.
+        // https://github.com/elastic/elasticsearch/issues/105821
+        boolean isSynthetic = name().startsWith(SYNTHETIC_ATTRIBUTE_NAME_PREFIX);
+
+        if (isSynthetic == false) {
+            return name();
+        }
+        return Strings.hasText(path) ? path + "." + field.getName() : field.getName();
+    }
+
     public String qualifiedPath() {
         // return only the qualifier is there's no path
         return qualifier() != null ? qualifier() + (Strings.hasText(path) ? "." + path : StringUtils.EMPTY) : path;

x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec

@@ -210,6 +210,20 @@ sample_data_str | 2023-10-23T12:27:28.948Z  |  2764889              |  Connected
 sample_data_str | 2023-10-23T12:15:03.360Z  |  3450233              |  Connected to 10.1.0.3
 ;
 
+multiIndexSortIpString
+required_capability: union_types
+required_capability: casting_operator
+required_capability: union_types_remove_fields
+
+FROM sample_data, sample_data_str
+| SORT client_ip::ip
+| LIMIT 1
+;
+
+@timestamp:date           |  client_ip:null  |  event_duration:long  |  message:keyword
+2023-10-23T13:33:34.937Z  |  null            |  1232382              |  Disconnected
+;
+
 multiIndexIpStringStats
 required_capability: union_types
 required_capability: casting_operator

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

@@ -133,7 +133,12 @@ public enum Cap {
          * Fix a parsing issue where numbers below Long.MIN_VALUE threw an exception instead of parsing as doubles.
          * see <a href="https://github.com/elastic/elasticsearch/issues/104323"> Parsing large numbers is inconsistent #104323 </a>
          */
-        FIX_PARSING_LARGE_NEGATIVE_NUMBERS;
+        FIX_PARSING_LARGE_NEGATIVE_NUMBERS,
+
+        /**
+         * Fix for union-types when sorting a type-casted field. We changed how we remove synthetic union-types fields.
+         */
+        UNION_TYPES_REMOVE_FIELDS;
 
         private final boolean snapshotOnly;
 

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java

@@ -39,6 +39,7 @@
 import org.elasticsearch.xpack.esql.core.plan.TableIdentifier;
 import org.elasticsearch.xpack.esql.core.rule.ParameterizedRule;
 import org.elasticsearch.xpack.esql.core.rule.ParameterizedRuleExecutor;
+import org.elasticsearch.xpack.esql.core.rule.Rule;
 import org.elasticsearch.xpack.esql.core.rule.RuleExecutor;
 import org.elasticsearch.xpack.esql.core.session.Configuration;
 import org.elasticsearch.xpack.esql.core.tree.Source;
@@ -60,6 +61,7 @@
 import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.DateTimeArithmeticOperation;
 import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.EsqlArithmeticOperation;
 import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.In;
+import org.elasticsearch.xpack.esql.optimizer.rules.SubstituteSurrogates;
 import org.elasticsearch.xpack.esql.plan.logical.Aggregate;
 import org.elasticsearch.xpack.esql.plan.logical.Drop;
 import org.elasticsearch.xpack.esql.plan.logical.Enrich;
@@ -139,7 +141,13 @@ public class Analyzer extends ParameterizedRuleExecutor<LogicalPlan, AnalyzerCon
             new ResolveUnionTypes(),  // Must be after ResolveRefs, so union types can be found
             new ImplicitCasting()
         );
-        var finish = new Batch<>("Finish Analysis", Limiter.ONCE, new AddImplicitLimit(), new UnresolveUnionTypes());
+        var finish = new Batch<>(
+            "Finish Analysis",
+            Limiter.ONCE,
+            new AddImplicitLimit(),
+            new UnresolveUnionTypes(),
+            new DropSyntheticUnionTypeAttributes()
+        );
         rules = List.of(init, resolution, finish);
     }
 
@@ -217,13 +225,13 @@ private static List<Attribute> mappingAsAttributes(Source source, Map<String, Es
         return list;
     }
 
-    private static void mappingAsAttributes(List<Attribute> list, Source source, String parentName, Map<String, EsField> mapping) {
+    private static void mappingAsAttributes(List<Attribute> list, Source source, FieldAttribute parent, Map<String, EsField> mapping) {
         for (Map.Entry<String, EsField> entry : mapping.entrySet()) {
             String name = entry.getKey();
             EsField t = entry.getValue();
 
             if (t != null) {
-                name = parentName == null ? name : parentName + "." + name;
+                name = parent == null ? name : parent.fieldName() + "." + name;
                 var fieldProperties = t.getProperties();
                 var type = t.getDataType().widenSmallNumeric();
                 // due to a bug also copy the field since the Attribute hierarchy extracts the data type
@@ -232,19 +240,16 @@ private static void mappingAsAttributes(List<Attribute> list, Source source, Str
                     t = new EsField(t.getName(), type, t.getProperties(), t.isAggregatable(), t.isAlias());
                 }
 
+                FieldAttribute attribute = t instanceof UnsupportedEsField uef
+                    ? new UnsupportedAttribute(source, name, uef)
+                    : new FieldAttribute(source, parent, name, t);
                 // primitive branch
                 if (EsqlDataTypes.isPrimitive(type)) {
-                    Attribute attribute;
-                    if (t instanceof UnsupportedEsField uef) {
-                        attribute = new UnsupportedAttribute(source, name, uef);
-                    } else {
-                        attribute = new FieldAttribute(source, null, name, t);
-                    }
                     list.add(attribute);
                 }
                 // allow compound object even if they are unknown (but not NESTED)
                 if (type != NESTED && fieldProperties.isEmpty() == false) {
-                    mappingAsAttributes(list, source, name, fieldProperties);
+                    mappingAsAttributes(list, source, attribute, fieldProperties);
                 }
             }
         }
@@ -1104,23 +1109,21 @@ protected LogicalPlan doRule(LogicalPlan plan) {
                 plan = plan.transformExpressionsOnly(UnresolvedAttribute.class, ua -> resolveAttribute(ua, resolved));
             }
 
-            // Otherwise drop the converted attributes after the alias function, as they are only needed for this function, and
-            // the original version of the attribute should still be seen as unconverted.
-            plan = dropConvertedAttributes(plan, unionFieldAttributes);
-
             // And add generated fields to EsRelation, so these new attributes will appear in the OutputExec of the Fragment
             // and thereby get used in FieldExtractExec
             plan = plan.transformDown(EsRelation.class, esr -> {
-                List<Attribute> output = esr.output();
                 List<Attribute> missing = new ArrayList<>();
                 for (FieldAttribute fa : unionFieldAttributes) {
-                    if (output.stream().noneMatch(a -> a.id().equals(fa.id()))) {
+                    // Using outputSet().contains looks by NameId, resp. uses semanticEquals.
+                    if (esr.outputSet().contains(fa) == false) {
                         missing.add(fa);
                     }
                 }
+
                 if (missing.isEmpty() == false) {
-                    output.addAll(missing);
-                    return new EsRelation(esr.source(), esr.index(), output, esr.indexMode(), esr.frozen());
+                    List<Attribute> newOutput = new ArrayList<>(esr.output());
+                    newOutput.addAll(missing);
+                    return new EsRelation(esr.source(), esr.index(), newOutput, esr.indexMode(), esr.frozen());
                 }
                 return esr;
             });
@@ -1136,17 +1139,6 @@ private Expression resolveAttribute(UnresolvedAttribute ua, Map<Attribute, Expre
             };
         }
 
-        private LogicalPlan dropConvertedAttributes(LogicalPlan plan, List<FieldAttribute> unionFieldAttributes) {
-            List<NamedExpression> projections = new ArrayList<>(plan.output());
-            for (var e : unionFieldAttributes) {
-                projections.removeIf(p -> p.id().equals(e.id()));
-            }
-            if (projections.size() != plan.output().size()) {
-                return new EsqlProject(plan.source(), plan, projections);
-            }
-            return plan;
-        }
-
         private Expression resolveConvertFunction(AbstractConvertFunction convert, List<FieldAttribute> unionFieldAttributes) {
             if (convert.field() instanceof FieldAttribute fa && fa.field() instanceof InvalidMappedField imf) {
                 HashMap<TypeResolutionKey, Expression> typeResolutions = new HashMap<>();
@@ -1175,7 +1167,13 @@ private Expression createIfDoesNotAlreadyExist(
             MultiTypeEsField resolvedField,
             List<FieldAttribute> unionFieldAttributes
         ) {
-            var unionFieldAttribute = new FieldAttribute(fa.source(), fa.name(), resolvedField);  // Generates new ID for the field
+            // Generate new ID for the field and suffix it with the data type to maintain unique attribute names.
+            String unionTypedFieldName = SubstituteSurrogates.rawTemporaryName(
+                fa.name(),
+                "converted_to",
+                resolvedField.getDataType().typeName()
+            );
+            FieldAttribute unionFieldAttribute = new FieldAttribute(fa.source(), fa.parent(), unionTypedFieldName, resolvedField);
             int existingIndex = unionFieldAttributes.indexOf(unionFieldAttribute);
             if (existingIndex >= 0) {
                 // Do not generate multiple name/type combinations with different IDs
@@ -1235,4 +1233,30 @@ static Attribute checkUnresolved(FieldAttribute fa) {
             return fa;
         }
     }
+
+    /**
+     * {@link ResolveUnionTypes} creates new, synthetic attributes for union types:
+     * If {@code client_ip} is present in 2 indices, once with type {@code ip} and once with type {@code keyword},
+     * using {@code EVAL x = to_ip(client_ip)} will create a single attribute @{code $$client_ip$converted_to$ip}.
+     * This should not spill into the query output, so we drop such attributes at the end.
+     */
+    private static class DropSyntheticUnionTypeAttributes extends Rule<LogicalPlan, LogicalPlan> {
+        public LogicalPlan apply(LogicalPlan plan) {
+            List<Attribute> output = plan.output();
+            List<Attribute> newOutput = new ArrayList<>(output.size());
+
+            for (Attribute attr : output) {
+                // TODO: this should really use .synthetic()
+                // https://github.com/elastic/elasticsearch/issues/105821
+                if (attr.name().startsWith(FieldAttribute.SYNTHETIC_ATTRIBUTE_NAME_PREFIX) == false) {
+                    newOutput.add(attr);
+                }
+            }
+
+            if (newOutput.size() != output.size()) {
+                return new Project(Source.EMPTY, plan, newOutput);
+            }
+            return plan;
+        }
+    }
 }

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/UnsupportedAttribute.java

@@ -104,6 +104,13 @@ public UnsupportedEsField field() {
         return (UnsupportedEsField) super.field();
     }
 
+    @Override
+    public String fieldName() {
+        // The super fieldName uses parents to compute the path; this class ignores parents, so we need to rely on the name instead.
+        // Using field().getName() would be wrong: for subfields like parent.subfield that would return only the last part, subfield.
+        return name();
+    }
+
     @Override
     protected NodeInfo<FieldAttribute> info() {
         return NodeInfo.create(this, UnsupportedAttribute::new, name(), field(), hasCustomMessage ? message : null, id());

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java

@@ -140,7 +140,8 @@ else if (plan instanceof Project project) {
                 Map<DataType, Alias> nullLiteral = Maps.newLinkedHashMapWithExpectedSize(DataType.types().size());
 
                 for (NamedExpression projection : projections) {
-                    if (projection instanceof FieldAttribute f && stats.exists(f.qualifiedName()) == false) {
+                    // Do not use the attribute name, this can deviate from the field name for union types.
+                    if (projection instanceof FieldAttribute f && stats.exists(f.fieldName()) == false) {
                         DataType dt = f.dataType();
                         Alias nullAlias = nullLiteral.get(f.dataType());
                         // save the first field as null (per datatype)
@@ -170,7 +171,8 @@ else if (plan instanceof Project project) {
                 || plan instanceof TopN) {
                     plan = plan.transformExpressionsOnlyUp(
                         FieldAttribute.class,
-                        f -> stats.exists(f.qualifiedName()) ? f : Literal.of(f, null)
+                        // Do not use the attribute name, this can deviate from the field name for union types.
+                        f -> stats.exists(f.fieldName()) ? f : Literal.of(f, null)
                     );
                 }
 

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/SubstituteSurrogates.java

@@ -14,6 +14,7 @@
 import org.elasticsearch.xpack.esql.core.expression.EmptyAttribute;
 import org.elasticsearch.xpack.esql.core.expression.Expression;
 import org.elasticsearch.xpack.esql.core.expression.Expressions;
+import org.elasticsearch.xpack.esql.core.expression.FieldAttribute;
 import org.elasticsearch.xpack.esql.core.expression.NamedExpression;
 import org.elasticsearch.xpack.esql.expression.SurrogateExpression;
 import org.elasticsearch.xpack.esql.expression.function.aggregate.AggregateFunction;
@@ -140,7 +141,7 @@ public static String temporaryName(Expression inner, Expression outer, int suffi
     }
 
     public static String rawTemporaryName(String inner, String outer, String suffix) {
-        return "$$" + inner + "$" + outer + "$" + suffix;
+        return FieldAttribute.SYNTHETIC_ATTRIBUTE_NAME_PREFIX + inner + "$" + outer + "$" + suffix;
     }
 
     static int TO_STRING_LIMIT = 16;

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/EsRelation.java

@@ -98,7 +98,7 @@ public boolean expressionsResolved() {
 
     @Override
     public int hashCode() {
-        return Objects.hash(index, indexMode, frozen);
+        return Objects.hash(index, indexMode, frozen, attrs);
     }
 
     @Override
@@ -112,7 +112,10 @@ public boolean equals(Object obj) {
         }
 
         EsRelation other = (EsRelation) obj;
-        return Objects.equals(index, other.index) && indexMode == other.indexMode() && frozen == other.frozen;
+        return Objects.equals(index, other.index)
+            && indexMode == other.indexMode()
+            && frozen == other.frozen
+            && Objects.equals(attrs, other.attrs);
     }
 
     @Override

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsPhysicalOperationProviders.java

@@ -117,7 +117,8 @@ public final PhysicalOperation fieldExtractPhysicalOperation(FieldExtractExec fi
             DataType dataType = attr.dataType();
             MappedFieldType.FieldExtractPreference fieldExtractPreference = PlannerUtils.extractPreference(docValuesAttrs.contains(attr));
             ElementType elementType = PlannerUtils.toElementType(dataType, fieldExtractPreference);
-            String fieldName = attr.name();
+            // Do not use the field attribute name, this can deviate from the field name for union types.
+            String fieldName = attr instanceof FieldAttribute fa ? fa.fieldName() : attr.name();
             boolean isUnsupported = dataType == DataType.UNSUPPORTED;
             IntFunction<BlockLoader> loader = s -> getBlockLoaderFor(s, fieldName, isUnsupported, fieldExtractPreference, unionTypes);
             fields.add(new ValuesSourceReaderOperator.FieldInfo(fieldName, elementType, loader));
@@ -235,8 +236,10 @@ public final Operator.OperatorFactory ordinalGroupingOperatorFactory(
         // Costin: why are they ready and not already exposed in the layout?
         boolean isUnsupported = attrSource.dataType() == DataType.UNSUPPORTED;
         var unionTypes = findUnionTypes(attrSource);
+        // Do not use the field attribute name, this can deviate from the field name for union types.
+        String fieldName = attrSource instanceof FieldAttribute fa ? fa.fieldName() : attrSource.name();
         return new OrdinalsGroupingOperator.OrdinalsGroupingOperatorFactory(
-            shardIdx -> getBlockLoaderFor(shardIdx, attrSource.name(), isUnsupported, NONE, unionTypes),
+            shardIdx -> getBlockLoaderFor(shardIdx, fieldName, isUnsupported, NONE, unionTypes),
             vsShardContexts,
             groupElementType,
             docChannel,