ESQL: Fix FieldAttribute name usage in InferNonNullAggConstraint (elastic#128910)

* Fix InferNonNullAggConstraint with union types
* Begin fixing LucenePushdownPredicates with union types
* Introduce a dedicated wrapper record FieldName to be used where field names are really required.

The fixes consist of using FieldAttribute.fieldName() instead of .name() or .field().name(). .name() can be some temporary string unrelated to the actual name of the Lucene index field, whereas .field().name() doesn't know about parent fields; .fieldName() gives the full field name (from the root of the document).

The biggest offender of such misuse is SearchStats; make this always require a FieldName, not a String - and make FieldAttribute#fieldName handily return an instance of FieldName so users of SearchStats don't accidentally use the return value of FieldAttribute#name.

Author: alex-spies

docs/changelog/128910.yaml

@@ -0,0 +1,5 @@
+pr: 128910
+summary: Fix `FieldAttribute` name usage in `InferNonNullAggConstraint`
+area: ES|QL
+type: bug
+issues: []

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

@@ -27,14 +27,24 @@
 
 /**
  * Attribute for an ES field.
- * To differentiate between the different type of fields this class offers:
- * - name - the fully qualified name (foo.bar.tar)
- * - path - the path pointing to the field name (foo.bar)
- * - parent - the immediate parent of the field; useful for figuring out the type of field (nested vs object)
- * - nestedParent - if nested, what's the parent (which might not be the immediate one)
+ * This class offers:
+ * - name - the name of the attribute, but not necessarily of the field.
+ * - The raw EsField representing the field; for parent.child.grandchild this is just grandchild.
+ * - parentName - the full path to the immediate parent of the field, e.g. parent.child (without .grandchild)
+ *
+ * To adequately represent e.g. union types, the name of the attribute can be altered because we may have multiple synthetic field
+ * attributes that really belong to the same underlying field. For instance, if a multi-typed field is used both as {@code field::string}
+ * and {@code field::ip}, we'll generate 2 field attributes called {@code $$field$converted_to$string} and {@code $$field$converted_to$ip}
+ * but still referring to the same underlying field.
  */
 public class FieldAttribute extends TypedAttribute {
 
+    /**
+     * A field name, as found in the mapping. Includes the whole path from the root of the document.
+     * Implemented as a wrapper around {@link String} to distinguish from the attribute name (which sometimes differs!) at compile time.
+     */
+    public record FieldName(String string) {};
+
     static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(
         Attribute.class,
         "FieldAttribute",
@@ -43,6 +53,7 @@ public class FieldAttribute extends TypedAttribute {
 
     private final String parentName;
     private final EsField field;
+    protected FieldName lazyFieldName;
 
     public FieldAttribute(Source source, String name, EsField field) {
         this(source, null, name, field);
@@ -136,15 +147,19 @@ public String parentName() {
     /**
      * 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.
-        // Because until 8.15, we couldn't set `synthetic` to true due to a bug, in that version such FieldAttributes are marked by their
-        // name starting with `$$`.
-        if ((synthetic() || name().startsWith(SYNTHETIC_ATTRIBUTE_NAME_PREFIX)) == false) {
-            return name();
+    public FieldName fieldName() {
+        if (lazyFieldName == null) {
+            // 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.
+            // Because until 8.15, we couldn't set `synthetic` to true due to a bug, in that version such FieldAttributes are marked by
+            // their
+            // name starting with `$$`.
+            if ((synthetic() || name().startsWith(SYNTHETIC_ATTRIBUTE_NAME_PREFIX)) == false) {
+                lazyFieldName = new FieldName(name());
+            }
+            lazyFieldName = new FieldName(Strings.hasText(parentName) ? parentName + "." + field.getName() : field.getName());
         }
-        return Strings.hasText(parentName) ? parentName + "." + field.getName() : field.getName();
+        return lazyFieldName;
     }
 
     public EsField.Exact getExactInfo() {

x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/EsField.java

@@ -117,7 +117,7 @@ public String getWriteableName() {
     }
 
     /**
-     * Returns the field path
+     * Returns the simple name, but not the full field path. The latter requires knowing the path of the parent field.
      */
     public String getName() {
         return name;

x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java

@@ -52,6 +52,7 @@
 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.ReferenceAttribute;
@@ -248,22 +249,22 @@ public static EsRelation relation() {
     public static class TestSearchStats implements SearchStats {
 
         @Override
-        public boolean exists(String field) {
+        public boolean exists(FieldName field) {
             return true;
         }
 
         @Override
-        public boolean isIndexed(String field) {
+        public boolean isIndexed(FieldName field) {
             return exists(field);
         }
 
         @Override
-        public boolean hasDocValues(String field) {
+        public boolean hasDocValues(FieldName field) {
             return exists(field);
         }
 
         @Override
-        public boolean hasExactSubfield(String field) {
+        public boolean hasExactSubfield(FieldName field) {
             return exists(field);
         }
 
@@ -273,32 +274,32 @@ public long count() {
         }
 
         @Override
-        public long count(String field) {
+        public long count(FieldName field) {
             return exists(field) ? -1 : 0;
         }
 
         @Override
-        public long count(String field, BytesRef value) {
+        public long count(FieldName field, BytesRef value) {
             return exists(field) ? -1 : 0;
         }
 
         @Override
-        public byte[] min(String field, DataType dataType) {
+        public byte[] min(FieldName field, DataType dataType) {
             return null;
         }
 
         @Override
-        public byte[] max(String field, DataType dataType) {
+        public byte[] max(FieldName field, DataType dataType) {
             return null;
         }
 
         @Override
-        public boolean isSingleValue(String field) {
+        public boolean isSingleValue(FieldName field) {
             return false;
         }
 
         @Override
-        public boolean canUseEqualityOnSyntheticSourceDelegate(String name, String value) {
+        public boolean canUseEqualityOnSyntheticSourceDelegate(FieldName name, String value) {
             return false;
         }
     }
@@ -349,23 +350,23 @@ private boolean isConfigationSet(Config config, String field) {
         }
 
         @Override
-        public boolean exists(String field) {
-            return isConfigationSet(Config.EXISTS, field);
+        public boolean exists(FieldName field) {
+            return isConfigationSet(Config.EXISTS, field.string());
         }
 
         @Override
-        public boolean isIndexed(String field) {
-            return isConfigationSet(Config.INDEXED, field);
+        public boolean isIndexed(FieldName field) {
+            return isConfigationSet(Config.INDEXED, field.string());
         }
 
         @Override
-        public boolean hasDocValues(String field) {
-            return isConfigationSet(Config.DOC_VALUES, field);
+        public boolean hasDocValues(FieldName field) {
+            return isConfigationSet(Config.DOC_VALUES, field.string());
         }
 
         @Override
-        public boolean hasExactSubfield(String field) {
-            return isConfigationSet(Config.EXACT_SUBFIELD, field);
+        public boolean hasExactSubfield(FieldName field) {
+            return isConfigationSet(Config.EXACT_SUBFIELD, field.string());
         }
 
         @Override
@@ -500,8 +501,8 @@ private static SearchStats fieldMatchingExistOrMissing(boolean exists, String...
             private final Set<String> fields = Set.of(names);
 
             @Override
-            public boolean exists(String field) {
-                return fields.contains(field) == exists;
+            public boolean exists(FieldName field) {
+                return fields.contains(field.string()) == exists;
             }
         };
     }

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

@@ -1325,6 +1325,19 @@ count:long  |  message:keyword
 3           |  Connected to 10.1.0.3
 ;
 
+multiIndexStatsOfMultiTypedField
+required_capability: union_types
+required_capability: casting_operator
+required_capability: union_types_numeric_widening
+
+FROM apps, apps_short
+| STATS s = sum(id::integer)
+;
+
+s:long
+210
+;
+
 multiIndexMultiColumnTypesRename
 required_capability: union_types
 required_capability: index_metadata_field

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

@@ -1744,7 +1744,7 @@ private Expression resolveConvertFunction(ConvertFunction convert, List<FieldAtt
                             indexToConversionExpressions.put(indexName, newConvertFunction);
                         }
                         MultiTypeEsField multiTypeEsField = new MultiTypeEsField(
-                            fa.fieldName(),
+                            fa.fieldName().string(),
                             convertExpression.dataType(),
                             false,
                             indexToConversionExpressions

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

@@ -123,10 +123,13 @@ public UnsupportedEsField 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();
+    public FieldName fieldName() {
+        if (lazyFieldName == null) {
+            // 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.
+            lazyFieldName = new FieldName(name());
+        }
+        return lazyFieldName;
     }
 
     @Override

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/local/InferNonNullAggConstraint.java

@@ -25,7 +25,7 @@
 
 /**
  * The vast majority of aggs ignore null entries - this rule adds a pushable filter, as it is cheap
- * to execute, to filter this entries out to begin with.
+ * to execute, to filter these entries out to begin with.
  * STATS x = min(a), y = sum(b)
  * becomes
  * | WHERE a IS NOT NULL OR b IS NOT NULL
@@ -55,7 +55,7 @@ protected LogicalPlan rule(Aggregate aggregate, LocalLogicalOptimizerContext con
                 Expression field = af.field();
                 // ignore literals (e.g. COUNT(1))
                 // make sure the field exists at the source and is indexed (not runtime)
-                if (field.foldable() == false && field instanceof FieldAttribute fa && stats.isIndexed(fa.name())) {
+                if (field.foldable() == false && field instanceof FieldAttribute fa && stats.isIndexed(fa.fieldName())) {
                     nonNullAggFields.add(field);
                 } else {
                     // otherwise bail out since unless disjunction needs to cover _all_ fields, things get filtered out

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/LucenePushdownPredicates.java

@@ -126,32 +126,36 @@ public boolean canUseEqualityOnSyntheticSourceDelegate(FieldAttribute attr, Stri
     };
 
     /**
-     * If we have access to SearchStats over a collection of shards, we can make more fine-grained decisions about what can be pushed down.
-     * This should open up more opportunities for lucene pushdown.
+     * If we have access to {@link SearchStats} over a collection of shards, we can make more fine-grained decisions about what can be
+     * pushed down. This should open up more opportunities for lucene pushdown.
      */
     static LucenePushdownPredicates from(SearchStats stats) {
+        // TODO: use FieldAttribute#fieldName, otherwise this doesn't apply to field attributes used for union types.
+        // C.f. https://github.com/elastic/elasticsearch/issues/128905
         return new LucenePushdownPredicates() {
             @Override
             public boolean hasExactSubfield(FieldAttribute attr) {
-                return stats.hasExactSubfield(attr.name());
+                return stats.hasExactSubfield(new FieldAttribute.FieldName(attr.name()));
             }
 
             @Override
             public boolean isIndexedAndHasDocValues(FieldAttribute attr) {
                 // We still consider the value of isAggregatable here, because some fields like ScriptFieldTypes are always aggregatable
                 // But this could hide issues with fields that are not indexed but are aggregatable
                 // This is the original behaviour for ES|QL, but is it correct?
-                return attr.field().isAggregatable() || stats.isIndexed(attr.name()) && stats.hasDocValues(attr.name());
+                return attr.field().isAggregatable()
+                    || stats.isIndexed(new FieldAttribute.FieldName(attr.name()))
+                        && stats.hasDocValues(new FieldAttribute.FieldName(attr.name()));
             }
 
             @Override
             public boolean isIndexed(FieldAttribute attr) {
-                return stats.isIndexed(attr.name());
+                return stats.isIndexed(new FieldAttribute.FieldName(attr.name()));
             }
 
             @Override
             public boolean canUseEqualityOnSyntheticSourceDelegate(FieldAttribute attr, String value) {
-                return stats.canUseEqualityOnSyntheticSourceDelegate(attr.field().getName(), value);
+                return stats.canUseEqualityOnSyntheticSourceDelegate(attr.fieldName(), value);
             }
         };
     }

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushStatsToSource.java

@@ -97,7 +97,7 @@ private Tuple<List<Attribute>, List<EsStatsQueryExec.Stat>> pushableStats(
                                 if (target instanceof FieldAttribute fa) {
                                     var fName = fa.fieldName();
                                     if (context.searchStats().isSingleValue(fName)) {
-                                        fieldName = fName;
+                                        fieldName = fName.string();
                                         query = QueryBuilders.existsQuery(fieldName);
                                     }
                                 }