QL: Preserve subfields for invalid types (elastic#100875)

In certain scenarios, a field can be mapped both as a primitive and
 object, causing it to be marked as unsupported, losing any potential
 subfields that might have been discovered before.
This commit preserve them to avoid subfields from being incorrectly
 reported as missing.

Fix elastic#100869

docs/changelog/100875.yaml

@@ -0,0 +1,6 @@
+pr: 100875
+summary: Preserve subfields for unsupported types
+area: "Query Languages"
+type: bug
+issues:
+ - 100869

x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/EsqlActionIT.java

@@ -1220,6 +1220,16 @@ public void testUnsupportedTypesOrdinalGrouping() {
         }
     }
 
+    public void testFilterNestedFields() {
+        assertAcked(client().admin().indices().prepareCreate("index-1").setMapping("file.name", "type=keyword"));
+        assertAcked(client().admin().indices().prepareCreate("index-2").setMapping("file", "type=keyword"));
+        try (var resp = run("from index-1,index-2 | where file.name is not null")) {
+            var valuesList = getValuesList(resp);
+            assertEquals(2, resp.columns().size());
+            assertEquals(0, valuesList.size());
+        }
+    }
+
     private void createNestedMappingIndex(String indexName) throws IOException {
         XContentBuilder builder = JsonXContent.contentBuilder();
         builder.startObject();

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/io/stream/PlanNamedTypes.java

@@ -956,12 +956,17 @@ static void writeDateEsField(PlanStreamOutput out, DateEsField dateEsField) thro
     }
 
     static InvalidMappedField readInvalidMappedField(PlanStreamInput in) throws IOException {
-        return new InvalidMappedField(in.readString(), in.readString());
+        return new InvalidMappedField(
+            in.readString(),
+            in.readString(),
+            in.readImmutableMap(StreamInput::readString, readerFromPlanReader(PlanStreamInput::readEsFieldNamed))
+        );
     }
 
     static void writeInvalidMappedField(PlanStreamOutput out, InvalidMappedField field) throws IOException {
         out.writeString(field.getName());
         out.writeString(field.errorMessage());
+        out.writeMap(field.getProperties(), (o, v) -> out.writeNamed(EsField.class, v));
     }
 
     static KeywordEsField readKeywordEsField(PlanStreamInput in) throws IOException {

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java

@@ -176,7 +176,15 @@ private <T> void preAnalyzeIndices(LogicalPlan parsed, ActionListener<IndexResol
             TableInfo tableInfo = preAnalysis.indices.get(0);
             TableIdentifier table = tableInfo.id();
             var fieldNames = fieldNames(parsed);
-            indexResolver.resolveAsMergedMapping(table.index(), fieldNames, false, Map.of(), listener, EsqlSession::specificValidity);
+            indexResolver.resolveAsMergedMapping(
+                table.index(),
+                fieldNames,
+                false,
+                Map.of(),
+                listener,
+                EsqlSession::specificValidity,
+                IndexResolver.PRESERVE_PROPERTIES
+            );
         } else {
             try {
                 // occurs when dealing with local relations (row a = 1)

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/type/EsqlDataTypeRegistryTests.java

@@ -56,7 +56,8 @@ private void resolve(String esTypeName, TimeSeriesParams.MetricType metricType,
             EsqlDataTypeRegistry.INSTANCE,
             "idx-*",
             caps,
-            EsqlSession::specificValidity
+            EsqlSession::specificValidity,
+            IndexResolver.PRESERVE_PROPERTIES
         );
 
         EsField f = resolution.get().mapping().get(fieldCap.getName());

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

@@ -55,6 +55,7 @@
 import java.util.Set;
 import java.util.TreeMap;
 import java.util.TreeSet;
+import java.util.function.BiConsumer;
 import java.util.function.BiFunction;
 import java.util.function.Function;
 import java.util.function.Supplier;
@@ -358,7 +359,26 @@ public void resolveAsMergedMapping(
         client.fieldCaps(
             fieldRequest,
             listener.delegateFailureAndWrap(
-                (l, response) -> l.onResponse(mergedMappings(typeRegistry, indexWildcard, response, specificValidityVerifier))
+                (l, response) -> l.onResponse(mergedMappings(typeRegistry, indexWildcard, response, specificValidityVerifier, null))
+            )
+        );
+    }
+
+    public void resolveAsMergedMapping(
+        String indexWildcard,
+        Set<String> fieldNames,
+        boolean includeFrozen,
+        Map<String, Object> runtimeMappings,
+        ActionListener<IndexResolution> listener,
+        BiFunction<String, Map<String, FieldCapabilities>, InvalidMappedField> specificValidityVerifier,
+        BiConsumer<EsField, InvalidMappedField> fieldUpdater
+
+    ) {
+        FieldCapabilitiesRequest fieldRequest = createFieldCapsRequest(indexWildcard, fieldNames, includeFrozen, runtimeMappings);
+        client.fieldCaps(
+            fieldRequest,
+            listener.delegateFailureAndWrap(
+                (l, response) -> l.onResponse(mergedMappings(typeRegistry, indexWildcard, response, specificValidityVerifier, fieldUpdater))
             )
         );
     }
@@ -369,13 +389,22 @@ public static IndexResolution mergedMappings(
         FieldCapabilitiesResponse fieldCapsResponse,
         BiFunction<String, Map<String, FieldCapabilities>, InvalidMappedField> specificValidityVerifier
     ) {
+        return mergedMappings(typeRegistry, indexPattern, fieldCapsResponse, specificValidityVerifier, null);
+    }
+
+    public static IndexResolution mergedMappings(
+        DataTypeRegistry typeRegistry,
+        String indexPattern,
+        FieldCapabilitiesResponse fieldCapsResponse,
+        BiFunction<String, Map<String, FieldCapabilities>, InvalidMappedField> specificValidityVerifier,
+        BiConsumer<EsField, InvalidMappedField> fieldUpdater
+    ) {
 
         if (fieldCapsResponse.getIndices().length == 0) {
             return IndexResolution.notFound(indexPattern);
         }
 
-        // merge all indices onto the same one
-        List<EsIndex> indices = buildIndices(typeRegistry, null, fieldCapsResponse, null, i -> indexPattern, (fieldName, types) -> {
+        BiFunction<String, Map<String, FieldCapabilities>, InvalidMappedField> validityVerifier = (fieldName, types) -> {
             InvalidMappedField f = specificValidityVerifier.apply(fieldName, types);
             if (f != null) {
                 return f;
@@ -431,7 +460,18 @@ public static IndexResolution mergedMappings(
 
             // everything checks
             return null;
-        });
+        };
+
+        // merge all indices onto the same one
+        List<EsIndex> indices = buildIndices(
+            typeRegistry,
+            null,
+            fieldCapsResponse,
+            null,
+            i -> indexPattern,
+            validityVerifier,
+            fieldUpdater
+        );
 
         if (indices.size() > 1) {
             throw new QlIllegalArgumentException(
@@ -454,7 +494,7 @@ public static IndexResolution mergedMappings(
         String indexPattern,
         FieldCapabilitiesResponse fieldCapsResponse
     ) {
-        return mergedMappings(typeRegistry, indexPattern, fieldCapsResponse, (fieldName, types) -> null);
+        return mergedMappings(typeRegistry, indexPattern, fieldCapsResponse, (fieldName, types) -> null, null);
     }
 
     private static EsField createField(
@@ -509,7 +549,7 @@ private static EsField createField(
 
         EsField esField = field.apply(fieldName);
 
-        if (parent != null && parent instanceof UnsupportedEsField unsupportedParent) {
+        if (parent instanceof UnsupportedEsField unsupportedParent) {
             String inherited = unsupportedParent.getInherited();
             String type = unsupportedParent.getOriginalType();
 
@@ -623,7 +663,7 @@ public static List<EsIndex> separateMappings(
         FieldCapabilitiesResponse fieldCaps,
         Map<String, List<AliasMetadata>> aliases
     ) {
-        return buildIndices(typeRegistry, javaRegex, fieldCaps, aliases, Function.identity(), (s, cap) -> null);
+        return buildIndices(typeRegistry, javaRegex, fieldCaps, aliases, Function.identity(), (s, cap) -> null, null);
     }
 
     private static class Fields {
@@ -641,7 +681,8 @@ private static List<EsIndex> buildIndices(
         FieldCapabilitiesResponse fieldCapsResponse,
         Map<String, List<AliasMetadata>> aliases,
         Function<String, String> indexNameProcessor,
-        BiFunction<String, Map<String, FieldCapabilities>, InvalidMappedField> validityVerifier
+        BiFunction<String, Map<String, FieldCapabilities>, InvalidMappedField> validityVerifier,
+        BiConsumer<EsField, InvalidMappedField> fieldUpdater
     ) {
 
         if ((fieldCapsResponse.getIndices() == null || fieldCapsResponse.getIndices().length == 0)
@@ -711,6 +752,25 @@ private static List<EsIndex> buildIndices(
                         EsField field = indexFields.flattedMapping.get(fieldName);
                         if (field == null || (invalidField != null && (field instanceof InvalidMappedField) == false)) {
                             createField(typeRegistry, fieldName, indexFields, fieldCaps, invalidField, typeCap);
+
+                            // In evolving mappings, it is possible for a field to be promoted to an object in new indices
+                            // meaning there are subfields associated with this *invalid* field.
+                            // index_A: file -> keyword
+                            // index_B: file -> object, file.name = keyword
+                            //
+                            // In the scenario above file is problematic but file.name is not. This scenario is addressed
+                            // below through the dedicated callback - copy the existing properties or drop them all together.
+                            // Note this applies for *invalid* fields (that have conflicts), not *unsupported* (those that cannot be read)
+                            // See https://github.com/elastic/elasticsearch/pull/100875
+
+                            // Postpone the call until is really needed
+                            if (fieldUpdater != null && field != null) {
+                                EsField newField = indexFields.flattedMapping.get(fieldName);
+
+                                if (newField != field) {
+                                    fieldUpdater.accept(field, (InvalidMappedField) newField);
+                                }
+                            }
                         }
                     }
                 }
@@ -772,7 +832,7 @@ private static void createField(
                     s,
                     typeCap.getType(),
                     typeCap.getMetricType(),
-                    emptyMap(),
+                    new TreeMap<>(),
                     typeCap.isAggregatable(),
                     isAliasFieldType.get()
                 )
@@ -915,4 +975,23 @@ private static Map<String, InvalidMappedField> getInvalidFieldsForAliases(
         // everything checks
         return emptyMap();
     }
+
+    /**
+     * Callback interface used when transitioning an already discovered EsField to an InvalidMapped one.
+     * By default, this interface is not used, meaning when a field is marked as invalid all its subfields
+     * are removed (are dropped).
+     * For cases where this is not desired, a different strategy can be employed such as keeping the properties:
+     * @see IndexResolver#PRESERVE_PROPERTIES
+     */
+    public interface ExistingFieldInvalidCallback extends BiConsumer<EsField, InvalidMappedField> {};
+
+    /**
+     * Preserve the properties (sub fields) of an existing field even when marking it as invalid.
+     */
+    public static ExistingFieldInvalidCallback PRESERVE_PROPERTIES = (oldField, newField) -> {
+        var oldProps = oldField.getProperties();
+        if (oldProps.size() > 0) {
+            newField.getProperties().putAll(oldProps);
+        }
+    };
 }

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/type/InvalidMappedField.java

@@ -9,9 +9,9 @@
 
 import org.elasticsearch.xpack.ql.QlIllegalArgumentException;
 
+import java.util.Map;
 import java.util.Objects;
-
-import static java.util.Collections.emptyMap;
+import java.util.TreeMap;
 
 /**
  * Representation of field mapped differently across indices.
@@ -21,14 +21,17 @@ public class InvalidMappedField extends EsField {
 
     private final String errorMessage;
 
-    public InvalidMappedField(String name, String errorMessage) {
-        super(name, DataTypes.UNSUPPORTED, emptyMap(), false);
+    public InvalidMappedField(String name, String errorMessage, Map<String, EsField> properties) {
+        super(name, DataTypes.UNSUPPORTED, properties, false);
         this.errorMessage = errorMessage;
     }
 
+    public InvalidMappedField(String name, String errorMessage) {
+        this(name, errorMessage, new TreeMap<String, EsField>());
+    }
+
     public InvalidMappedField(String name) {
-        super(name, DataTypes.UNSUPPORTED, emptyMap(), false);
-        this.errorMessage = StringUtils.EMPTY;
+        this(name, StringUtils.EMPTY, new TreeMap<String, EsField>());
     }
 
     public String errorMessage() {

x-pack/plugin/ql/src/test/resources/fc-incompatible-object-compatible-subfields.json

@@ -0,0 +1,48 @@
+{
+  "indices": [
+    "index-1",
+    "index-2"
+  ],
+  "fields": {
+    "file": {
+      "keyword": {
+        "type": "keyword",
+        "metadata_field": false,
+        "searchable": true,
+        "aggregatable": true,
+        "indices": [
+          "index-2"
+        ]
+      },
+      "object": {
+        "type": "object",
+        "metadata_field": false,
+        "searchable": false,
+        "aggregatable": false,
+        "indices": [
+          "index-1"
+        ]
+      }
+    },
+    "file.name": {
+      "keyword": {
+        "type": "keyword",
+        "metadata_field": false,
+        "searchable": true,
+        "aggregatable": true,
+        "indices": [
+          "index-1"
+        ]
+      },
+      "unmapped": {
+        "type": "unmapped",
+        "metadata_field": false,
+        "searchable": false,
+        "aggregatable": false,
+        "indices": [
+          "index-2"
+        ]
+      }
+    }
+  }
+}

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/index/IndexResolverTests.java

@@ -8,8 +8,14 @@
 
 import org.elasticsearch.action.fieldcaps.FieldCapabilities;
 import org.elasticsearch.action.fieldcaps.FieldCapabilitiesResponse;
+import org.elasticsearch.common.bytes.BytesReference;
+import org.elasticsearch.common.io.Streams;
 import org.elasticsearch.common.util.Maps;
+import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.xcontent.XContentParser;
+import org.elasticsearch.xcontent.XContentParserConfiguration;
+import org.elasticsearch.xcontent.XContentType;
 import org.elasticsearch.xpack.ql.index.EsIndex;
 import org.elasticsearch.xpack.ql.index.IndexResolution;
 import org.elasticsearch.xpack.ql.index.IndexResolver;
@@ -19,6 +25,8 @@
 import org.elasticsearch.xpack.ql.type.KeywordEsField;
 import org.elasticsearch.xpack.sql.type.SqlDataTypeRegistry;
 
+import java.io.IOException;
+import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -396,6 +404,42 @@ public void testIndexWithNoMapping() {
         assertTrue(mergedMappings("*", new String[] { "empty" }, versionFC).isValid());
     }
 
+    public void testMergeObjectIncompatibleTypes() throws Exception {
+        var response = readFieldCapsResponse("fc-incompatible-object-compatible-subfields.json");
+
+        IndexResolution resolution = IndexResolver.mergedMappings(
+            SqlDataTypeRegistry.INSTANCE,
+            "*",
+            response,
+            (fieldName, types) -> null,
+            IndexResolver.PRESERVE_PROPERTIES
+
+        );
+
+        assertTrue(resolution.isValid());
+        EsIndex esIndex = resolution.get();
+        assertEquals(Set.of("index-1", "index-2"), esIndex.concreteIndices());
+        EsField esField = esIndex.mapping().get("file");
+        assertEquals(InvalidMappedField.class, esField.getClass());
+
+        assertEquals(
+            "mapped as [2] incompatible types: [keyword] in [index-2], [object] in [index-1]",
+            ((InvalidMappedField) esField).errorMessage()
+        );
+
+        esField = esField.getProperties().get("name");
+        assertNotNull(esField);
+        assertEquals(esField.getDataType(), KEYWORD);
+        assertEquals(KeywordEsField.class, esField.getClass());
+    }
+
+    private static FieldCapabilitiesResponse readFieldCapsResponse(String resourceName) throws IOException {
+        InputStream stream = IndexResolverTests.class.getResourceAsStream("/" + resourceName);
+        BytesReference ref = Streams.readFully(stream);
+        XContentParser parser = XContentHelper.createParser(XContentParserConfiguration.EMPTY, ref, XContentType.JSON);
+        return FieldCapabilitiesResponse.fromXContent(parser);
+    }
+
     public static IndexResolution merge(EsIndex... indices) {
         return mergedMappings("*", Stream.of(indices).map(EsIndex::name).toArray(String[]::new), fromMappings(indices));
     }