Skip to content

Commit 7c48775

Browse files
costincostin
authored
QL: Preserve subfields for invalid types (#100875) (#100933)
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 #100869
1 parent 454cd35 commit 7c48775

File tree

9 files changed

+222
-18
lines changed

9 files changed

+222
-18
lines changed

‎docs/changelog/100875.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 100875
2+
summary: Preserve subfields for unsupported types
3+
area: "Query Languages"
4+
type: bug
5+
issues:
6+
- 100869

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1220,6 +1220,16 @@ public void testUnsupportedTypesOrdinalGrouping() {
12201220
}
12211221
}
12221222

1223+
public void testFilterNestedFields() {
1224+
assertAcked(client().admin().indices().prepareCreate("index-1").setMapping("file.name", "type=keyword"));
1225+
assertAcked(client().admin().indices().prepareCreate("index-2").setMapping("file", "type=keyword"));
1226+
try (var resp = run("from index-1,index-2 | where file.name is not null")) {
1227+
var valuesList = getValuesList(resp);
1228+
assertEquals(2, resp.columns().size());
1229+
assertEquals(0, valuesList.size());
1230+
}
1231+
}
1232+
12231233
private void createNestedMappingIndex(String indexName) throws IOException {
12241234
XContentBuilder builder = JsonXContent.contentBuilder();
12251235
builder.startObject();

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -956,12 +956,17 @@ static void writeDateEsField(PlanStreamOutput out, DateEsField dateEsField) thro
956956
}
957957

958958
static InvalidMappedField readInvalidMappedField(PlanStreamInput in) throws IOException {
959-
return new InvalidMappedField(in.readString(), in.readString());
959+
return new InvalidMappedField(
960+
in.readString(),
961+
in.readString(),
962+
in.readImmutableMap(StreamInput::readString, readerFromPlanReader(PlanStreamInput::readEsFieldNamed))
963+
);
960964
}
961965

962966
static void writeInvalidMappedField(PlanStreamOutput out, InvalidMappedField field) throws IOException {
963967
out.writeString(field.getName());
964968
out.writeString(field.errorMessage());
969+
out.writeMap(field.getProperties(), (o, v) -> out.writeNamed(EsField.class, v));
965970
}
966971

967972
static KeywordEsField readKeywordEsField(PlanStreamInput in) throws IOException {

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,15 @@ private <T> void preAnalyzeIndices(LogicalPlan parsed, ActionListener<IndexResol
176176
TableInfo tableInfo = preAnalysis.indices.get(0);
177177
TableIdentifier table = tableInfo.id();
178178
var fieldNames = fieldNames(parsed);
179-
indexResolver.resolveAsMergedMapping(table.index(), fieldNames, false, Map.of(), listener, EsqlSession::specificValidity);
179+
indexResolver.resolveAsMergedMapping(
180+
table.index(),
181+
fieldNames,
182+
false,
183+
Map.of(),
184+
listener,
185+
EsqlSession::specificValidity,
186+
IndexResolver.PRESERVE_PROPERTIES
187+
);
180188
} else {
181189
try {
182190
// occurs when dealing with local relations (row a = 1)

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,8 @@ private void resolve(String esTypeName, TimeSeriesParams.MetricType metricType,
5656
EsqlDataTypeRegistry.INSTANCE,
5757
"idx-*",
5858
caps,
59-
EsqlSession::specificValidity
59+
EsqlSession::specificValidity,
60+
IndexResolver.PRESERVE_PROPERTIES
6061
);
6162

6263
EsField f = resolution.get().mapping().get(fieldCap.getName());

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

Lines changed: 88 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@
5555
import java.util.Set;
5656
import java.util.TreeMap;
5757
import java.util.TreeSet;
58+
import java.util.function.BiConsumer;
5859
import java.util.function.BiFunction;
5960
import java.util.function.Function;
6061
import java.util.function.Supplier;
@@ -358,7 +359,26 @@ public void resolveAsMergedMapping(
358359
client.fieldCaps(
359360
fieldRequest,
360361
listener.delegateFailureAndWrap(
361-
(l, response) -> l.onResponse(mergedMappings(typeRegistry, indexWildcard, response, specificValidityVerifier))
362+
(l, response) -> l.onResponse(mergedMappings(typeRegistry, indexWildcard, response, specificValidityVerifier, null))
363+
)
364+
);
365+
}
366+
367+
public void resolveAsMergedMapping(
368+
String indexWildcard,
369+
Set<String> fieldNames,
370+
boolean includeFrozen,
371+
Map<String, Object> runtimeMappings,
372+
ActionListener<IndexResolution> listener,
373+
BiFunction<String, Map<String, FieldCapabilities>, InvalidMappedField> specificValidityVerifier,
374+
BiConsumer<EsField, InvalidMappedField> fieldUpdater
375+
376+
) {
377+
FieldCapabilitiesRequest fieldRequest = createFieldCapsRequest(indexWildcard, fieldNames, includeFrozen, runtimeMappings);
378+
client.fieldCaps(
379+
fieldRequest,
380+
listener.delegateFailureAndWrap(
381+
(l, response) -> l.onResponse(mergedMappings(typeRegistry, indexWildcard, response, specificValidityVerifier, fieldUpdater))
362382
)
363383
);
364384
}
@@ -369,13 +389,22 @@ public static IndexResolution mergedMappings(
369389
FieldCapabilitiesResponse fieldCapsResponse,
370390
BiFunction<String, Map<String, FieldCapabilities>, InvalidMappedField> specificValidityVerifier
371391
) {
392+
return mergedMappings(typeRegistry, indexPattern, fieldCapsResponse, specificValidityVerifier, null);
393+
}
394+
395+
public static IndexResolution mergedMappings(
396+
DataTypeRegistry typeRegistry,
397+
String indexPattern,
398+
FieldCapabilitiesResponse fieldCapsResponse,
399+
BiFunction<String, Map<String, FieldCapabilities>, InvalidMappedField> specificValidityVerifier,
400+
BiConsumer<EsField, InvalidMappedField> fieldUpdater
401+
) {
372402

373403
if (fieldCapsResponse.getIndices().length == 0) {
374404
return IndexResolution.notFound(indexPattern);
375405
}
376406

377-
// merge all indices onto the same one
378-
List<EsIndex> indices = buildIndices(typeRegistry, null, fieldCapsResponse, null, i -> indexPattern, (fieldName, types) -> {
407+
BiFunction<String, Map<String, FieldCapabilities>, InvalidMappedField> validityVerifier = (fieldName, types) -> {
379408
InvalidMappedField f = specificValidityVerifier.apply(fieldName, types);
380409
if (f != null) {
381410
return f;
@@ -431,7 +460,18 @@ public static IndexResolution mergedMappings(
431460

432461
// everything checks
433462
return null;
434-
});
463+
};
464+
465+
// merge all indices onto the same one
466+
List<EsIndex> indices = buildIndices(
467+
typeRegistry,
468+
null,
469+
fieldCapsResponse,
470+
null,
471+
i -> indexPattern,
472+
validityVerifier,
473+
fieldUpdater
474+
);
435475

436476
if (indices.size() > 1) {
437477
throw new QlIllegalArgumentException(
@@ -454,7 +494,7 @@ public static IndexResolution mergedMappings(
454494
String indexPattern,
455495
FieldCapabilitiesResponse fieldCapsResponse
456496
) {
457-
return mergedMappings(typeRegistry, indexPattern, fieldCapsResponse, (fieldName, types) -> null);
497+
return mergedMappings(typeRegistry, indexPattern, fieldCapsResponse, (fieldName, types) -> null, null);
458498
}
459499

460500
private static EsField createField(
@@ -509,7 +549,7 @@ private static EsField createField(
509549

510550
EsField esField = field.apply(fieldName);
511551

512-
if (parent != null && parent instanceof UnsupportedEsField unsupportedParent) {
552+
if (parent instanceof UnsupportedEsField unsupportedParent) {
513553
String inherited = unsupportedParent.getInherited();
514554
String type = unsupportedParent.getOriginalType();
515555

@@ -623,7 +663,7 @@ public static List<EsIndex> separateMappings(
623663
FieldCapabilitiesResponse fieldCaps,
624664
Map<String, List<AliasMetadata>> aliases
625665
) {
626-
return buildIndices(typeRegistry, javaRegex, fieldCaps, aliases, Function.identity(), (s, cap) -> null);
666+
return buildIndices(typeRegistry, javaRegex, fieldCaps, aliases, Function.identity(), (s, cap) -> null, null);
627667
}
628668

629669
private static class Fields {
@@ -641,7 +681,8 @@ private static List<EsIndex> buildIndices(
641681
FieldCapabilitiesResponse fieldCapsResponse,
642682
Map<String, List<AliasMetadata>> aliases,
643683
Function<String, String> indexNameProcessor,
644-
BiFunction<String, Map<String, FieldCapabilities>, InvalidMappedField> validityVerifier
684+
BiFunction<String, Map<String, FieldCapabilities>, InvalidMappedField> validityVerifier,
685+
BiConsumer<EsField, InvalidMappedField> fieldUpdater
645686
) {
646687

647688
if ((fieldCapsResponse.getIndices() == null || fieldCapsResponse.getIndices().length == 0)
@@ -711,6 +752,25 @@ private static List<EsIndex> buildIndices(
711752
EsField field = indexFields.flattedMapping.get(fieldName);
712753
if (field == null || (invalidField != null && (field instanceof InvalidMappedField) == false)) {
713754
createField(typeRegistry, fieldName, indexFields, fieldCaps, invalidField, typeCap);
755+
756+
// In evolving mappings, it is possible for a field to be promoted to an object in new indices
757+
// meaning there are subfields associated with this *invalid* field.
758+
// index_A: file -> keyword
759+
// index_B: file -> object, file.name = keyword
760+
//
761+
// In the scenario above file is problematic but file.name is not. This scenario is addressed
762+
// below through the dedicated callback - copy the existing properties or drop them all together.
763+
// Note this applies for *invalid* fields (that have conflicts), not *unsupported* (those that cannot be read)
764+
// See https://github.com/elastic/elasticsearch/pull/100875
765+
766+
// Postpone the call until is really needed
767+
if (fieldUpdater != null && field != null) {
768+
EsField newField = indexFields.flattedMapping.get(fieldName);
769+
770+
if (newField != field) {
771+
fieldUpdater.accept(field, (InvalidMappedField) newField);
772+
}
773+
}
714774
}
715775
}
716776
}
@@ -772,7 +832,7 @@ private static void createField(
772832
s,
773833
typeCap.getType(),
774834
typeCap.getMetricType(),
775-
emptyMap(),
835+
new TreeMap<>(),
776836
typeCap.isAggregatable(),
777837
isAliasFieldType.get()
778838
)
@@ -915,4 +975,23 @@ private static Map<String, InvalidMappedField> getInvalidFieldsForAliases(
915975
// everything checks
916976
return emptyMap();
917977
}
978+
979+
/**
980+
* Callback interface used when transitioning an already discovered EsField to an InvalidMapped one.
981+
* By default, this interface is not used, meaning when a field is marked as invalid all its subfields
982+
* are removed (are dropped).
983+
* For cases where this is not desired, a different strategy can be employed such as keeping the properties:
984+
* @see IndexResolver#PRESERVE_PROPERTIES
985+
*/
986+
public interface ExistingFieldInvalidCallback extends BiConsumer<EsField, InvalidMappedField> {};
987+
988+
/**
989+
* Preserve the properties (sub fields) of an existing field even when marking it as invalid.
990+
*/
991+
public static ExistingFieldInvalidCallback PRESERVE_PROPERTIES = (oldField, newField) -> {
992+
var oldProps = oldField.getProperties();
993+
if (oldProps.size() > 0) {
994+
newField.getProperties().putAll(oldProps);
995+
}
996+
};
918997
}

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

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@
99

1010
import org.elasticsearch.xpack.ql.QlIllegalArgumentException;
1111

12+
import java.util.Map;
1213
import java.util.Objects;
13-
14-
import static java.util.Collections.emptyMap;
14+
import java.util.TreeMap;
1515

1616
/**
1717
* Representation of field mapped differently across indices.
@@ -21,14 +21,17 @@ public class InvalidMappedField extends EsField {
2121

2222
private final String errorMessage;
2323

24-
public InvalidMappedField(String name, String errorMessage) {
25-
super(name, DataTypes.UNSUPPORTED, emptyMap(), false);
24+
public InvalidMappedField(String name, String errorMessage, Map<String, EsField> properties) {
25+
super(name, DataTypes.UNSUPPORTED, properties, false);
2626
this.errorMessage = errorMessage;
2727
}
2828

29+
public InvalidMappedField(String name, String errorMessage) {
30+
this(name, errorMessage, new TreeMap<String, EsField>());
31+
}
32+
2933
public InvalidMappedField(String name) {
30-
super(name, DataTypes.UNSUPPORTED, emptyMap(), false);
31-
this.errorMessage = StringUtils.EMPTY;
34+
this(name, StringUtils.EMPTY, new TreeMap<String, EsField>());
3235
}
3336

3437
public String errorMessage() {

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
{
2+
"indices": [
3+
"index-1",
4+
"index-2"
5+
],
6+
"fields": {
7+
"file": {
8+
"keyword": {
9+
"type": "keyword",
10+
"metadata_field": false,
11+
"searchable": true,
12+
"aggregatable": true,
13+
"indices": [
14+
"index-2"
15+
]
16+
},
17+
"object": {
18+
"type": "object",
19+
"metadata_field": false,
20+
"searchable": false,
21+
"aggregatable": false,
22+
"indices": [
23+
"index-1"
24+
]
25+
}
26+
},
27+
"file.name": {
28+
"keyword": {
29+
"type": "keyword",
30+
"metadata_field": false,
31+
"searchable": true,
32+
"aggregatable": true,
33+
"indices": [
34+
"index-1"
35+
]
36+
},
37+
"unmapped": {
38+
"type": "unmapped",
39+
"metadata_field": false,
40+
"searchable": false,
41+
"aggregatable": false,
42+
"indices": [
43+
"index-2"
44+
]
45+
}
46+
}
47+
}
48+
}

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

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,14 @@
88

99
import org.elasticsearch.action.fieldcaps.FieldCapabilities;
1010
import org.elasticsearch.action.fieldcaps.FieldCapabilitiesResponse;
11+
import org.elasticsearch.common.bytes.BytesReference;
12+
import org.elasticsearch.common.io.Streams;
1113
import org.elasticsearch.common.util.Maps;
14+
import org.elasticsearch.common.xcontent.XContentHelper;
1215
import org.elasticsearch.test.ESTestCase;
16+
import org.elasticsearch.xcontent.XContentParser;
17+
import org.elasticsearch.xcontent.XContentParserConfiguration;
18+
import org.elasticsearch.xcontent.XContentType;
1319
import org.elasticsearch.xpack.ql.index.EsIndex;
1420
import org.elasticsearch.xpack.ql.index.IndexResolution;
1521
import org.elasticsearch.xpack.ql.index.IndexResolver;
@@ -19,6 +25,8 @@
1925
import org.elasticsearch.xpack.ql.type.KeywordEsField;
2026
import org.elasticsearch.xpack.sql.type.SqlDataTypeRegistry;
2127

28+
import java.io.IOException;
29+
import java.io.InputStream;
2230
import java.util.ArrayList;
2331
import java.util.Arrays;
2432
import java.util.Collections;
@@ -396,6 +404,42 @@ public void testIndexWithNoMapping() {
396404
assertTrue(mergedMappings("*", new String[] { "empty" }, versionFC).isValid());
397405
}
398406

407+
public void testMergeObjectIncompatibleTypes() throws Exception {
408+
var response = readFieldCapsResponse("fc-incompatible-object-compatible-subfields.json");
409+
410+
IndexResolution resolution = IndexResolver.mergedMappings(
411+
SqlDataTypeRegistry.INSTANCE,
412+
"*",
413+
response,
414+
(fieldName, types) -> null,
415+
IndexResolver.PRESERVE_PROPERTIES
416+
417+
);
418+
419+
assertTrue(resolution.isValid());
420+
EsIndex esIndex = resolution.get();
421+
assertEquals(Set.of("index-1", "index-2"), esIndex.concreteIndices());
422+
EsField esField = esIndex.mapping().get("file");
423+
assertEquals(InvalidMappedField.class, esField.getClass());
424+
425+
assertEquals(
426+
"mapped as [2] incompatible types: [keyword] in [index-2], [object] in [index-1]",
427+
((InvalidMappedField) esField).errorMessage()
428+
);
429+
430+
esField = esField.getProperties().get("name");
431+
assertNotNull(esField);
432+
assertEquals(esField.getDataType(), KEYWORD);
433+
assertEquals(KeywordEsField.class, esField.getClass());
434+
}
435+
436+
private static FieldCapabilitiesResponse readFieldCapsResponse(String resourceName) throws IOException {
437+
InputStream stream = IndexResolverTests.class.getResourceAsStream("/" + resourceName);
438+
BytesReference ref = Streams.readFully(stream);
439+
XContentParser parser = XContentHelper.createParser(XContentParserConfiguration.EMPTY, ref, XContentType.JSON);
440+
return FieldCapabilitiesResponse.fromXContent(parser);
441+
}
442+
399443
public static IndexResolution merge(EsIndex... indices) {
400444
return mergedMappings("*", Stream.of(indices).map(EsIndex::name).toArray(String[]::new), fromMappings(indices));
401445
}

0 commit comments

Comments
 (0)