Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create aliases for PassThroughObjectMapper subfields in FieldTypeLookup #106829

Merged
merged 23 commits into from
May 17, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
009ee70
Alias PassThroughObjectMapper subfields in FieldTypeLookup
kkrik-es Mar 27, 2024
c54fd29
small refactor
kkrik-es Mar 27, 2024
6ec8699
conflict resolution
kkrik-es Mar 27, 2024
05a6016
add check for circular deps
kkrik-es Mar 28, 2024
cbd6076
Merge branch 'main' into fix/passthrough-no-alias
kkrik-es Mar 28, 2024
c084c31
use lexicographical ordering for conflicting aliases
kkrik-es Mar 28, 2024
5109d56
use lexicographical ordering for conflicting aliases
kkrik-es Mar 28, 2024
145ba0d
Merge branch 'main' into fix/passthrough-no-alias
kkrik-es Apr 4, 2024
d867e30
simplify loop producing aliases
kkrik-es Apr 4, 2024
0f9b08d
use priorities for passthrough deduplication
kkrik-es Apr 5, 2024
7f2dc2a
make priority param required
kkrik-es Apr 5, 2024
45aa214
add passthrough disclaimer
kkrik-es Apr 8, 2024
3cd0f1c
small refactor
kkrik-es Apr 8, 2024
ba3aecc
small refactor
kkrik-es Apr 8, 2024
a72f16d
revert refactor
kkrik-es Apr 8, 2024
201b882
Merge branch 'refs/heads/main' into fix/passthrough-no-alias
kkrik-es Apr 8, 2024
f700d59
Merge branch 'refs/heads/main' into fix/passthrough-no-alias
kkrik-es Apr 9, 2024
8553b99
Merge branch 'refs/heads/main' into fix/passthrough-no-alias
kkrik-es Apr 9, 2024
e1241c5
Merge branch 'refs/heads/main' into fix/passthrough-no-alias
kkrik-es Apr 25, 2024
879afde
Merge branch 'refs/heads/main' into fix/passthrough-no-alias
kkrik-es Apr 29, 2024
20ed13d
node feature for pass-through priotity
kkrik-es Apr 29, 2024
1a5823c
Merge branch 'refs/heads/main' into fix/passthrough-no-alias
kkrik-es May 10, 2024
f7e78aa
Merge branch 'refs/heads/main' into fix/passthrough-no-alias
kkrik-es May 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
import org.elasticsearch.action.admin.indices.template.put.TransportPutComposableIndexTemplateAction;
import org.elasticsearch.action.bulk.BulkRequest;
import org.elasticsearch.action.delete.DeleteRequest;
import org.elasticsearch.action.fieldcaps.FieldCapabilitiesRequest;
import org.elasticsearch.action.fieldcaps.FieldCapabilitiesResponse;
import org.elasticsearch.action.get.GetRequest;
import org.elasticsearch.action.index.IndexRequest;
import org.elasticsearch.action.search.SearchRequest;
Expand Down Expand Up @@ -197,31 +199,20 @@ public void testIndexingGettingAndSearching() throws Exception {
assertMap(attributes.get("pod.ip"), matchesMap().entry("type", "ip").entry("time_series_dimension", true));
assertMap(attributes.get("pod.uid"), matchesMap().entry("type", "keyword").entry("time_series_dimension", true));
assertMap(attributes.get("pod.name"), matchesMap().entry("type", "keyword").entry("time_series_dimension", true));
// alias field mappers:
assertMap(
ObjectPath.eval("properties.metricset", mapping),
matchesMap().entry("type", "alias").entry("path", "attributes.metricset")
);
assertMap(
ObjectPath.eval("properties.number.properties.long", mapping),
matchesMap().entry("type", "alias").entry("path", "attributes.number.long")
);
assertMap(
ObjectPath.eval("properties.number.properties.double", mapping),
matchesMap().entry("type", "alias").entry("path", "attributes.number.double")
);
assertMap(
ObjectPath.eval("properties.pod.properties", mapping),
matchesMap().extraOk().entry("name", matchesMap().entry("type", "alias").entry("path", "attributes.pod.name"))
);
assertMap(
ObjectPath.eval("properties.pod.properties", mapping),
matchesMap().extraOk().entry("uid", matchesMap().entry("type", "alias").entry("path", "attributes.pod.uid"))
);
assertMap(
ObjectPath.eval("properties.pod.properties", mapping),
matchesMap().extraOk().entry("ip", matchesMap().entry("type", "alias").entry("path", "attributes.pod.ip"))
);

FieldCapabilitiesResponse fieldCaps = client().fieldCaps(new FieldCapabilitiesRequest().fields("*").indices("k8s")).actionGet();
assertTrue(fieldCaps.getField("attributes.metricset").get("keyword").isDimension());
assertTrue(fieldCaps.getField("metricset").get("keyword").isDimension());
assertTrue(fieldCaps.getField("attributes.number.long").get("long").isDimension());
assertTrue(fieldCaps.getField("number.long").get("long").isDimension());
assertTrue(fieldCaps.getField("attributes.number.double").get("float").isDimension());
assertTrue(fieldCaps.getField("number.double").get("float").isDimension());
assertTrue(fieldCaps.getField("attributes.pod.ip").get("ip").isDimension());
assertTrue(fieldCaps.getField("pod.ip").get("ip").isDimension());
assertTrue(fieldCaps.getField("attributes.pod.uid").get("keyword").isDimension());
assertTrue(fieldCaps.getField("pod.uid").get("keyword").isDimension());
assertTrue(fieldCaps.getField("attributes.pod.name").get("keyword").isDimension());
assertTrue(fieldCaps.getField("pod.name").get("keyword").isDimension());
}

public void testIndexingGettingAndSearchingShrunkIndex() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ public void setup() throws Exception {
new MetadataFieldMapper[] { dtfm },
Collections.emptyMap()
);
MappingLookup mappingLookup = MappingLookup.fromMappers(mapping, List.of(dtfm, dateFieldMapper), List.of(), List.of());
MappingLookup mappingLookup = MappingLookup.fromMappers(mapping, List.of(dtfm, dateFieldMapper), List.of());
indicesService = DataStreamTestHelper.mockIndicesServices(mappingLookup);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,7 @@ dynamic templates - conflicting aliases:
type: passthrough
dynamic: true
time_series_dimension: true
superseded_by: attributes
dynamic_templates:
- counter_metric:
mapping:
Expand Down Expand Up @@ -391,7 +392,7 @@ dynamic templates - conflicting aliases:
filterA:
filter:
term:
dim: "C"
dim: A
aggs:
tsids:
terms:
Expand All @@ -410,7 +411,7 @@ dynamic templates - conflicting aliases:
filterA:
filter:
term:
attributes.dim: A
resource_attributes.dim: C
aggs:
tsids:
terms:
Expand Down Expand Up @@ -833,11 +834,39 @@ enable subobjects in passthrough object:
index:
number_of_shards: 1
mode: time_series
time_series:
start_time: 2023-08-31T13:03:08.138Z

mappings:
properties:
attributes:
type: passthrough
subobjects: true

---
passthrough objects with circular deps:
- skip:
version: " - 8.12.99"
reason: "Support for passthrough fields was added in 8.13"
- do:
catch: /Circular dependency between pass-through objects/
indices.put_index_template:
name: my-dynamic-template
body:
index_patterns: [k9s*]
data_stream: {}
template:
settings:
index:
number_of_shards: 1
mode: time_series

mappings:
properties:
attributes:
type: passthrough
superseded_by: system.attributes
resource.attributes:
type: passthrough
superseded_by: attributes
system.attributes:
type: passthrough
superseded_by: resource.attributes
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
Expand All @@ -38,9 +39,14 @@ final class FieldTypeLookup {

private final int maxParentPathDots;

FieldTypeLookup(Collection<FieldMapper> fieldMappers, Collection<FieldAliasMapper> fieldAliasMappers) {
this(fieldMappers, fieldAliasMappers, List.of(), List.of());
}

FieldTypeLookup(
Collection<FieldMapper> fieldMappers,
Collection<FieldAliasMapper> fieldAliasMappers,
Collection<PassThroughObjectMapper> passThroughMappers,
Collection<RuntimeField> runtimeFields
) {

Expand Down Expand Up @@ -86,6 +92,35 @@ final class FieldTypeLookup {
}
}

// Pass-though subfields can be referenced without the prefix corresponding to the
// PassThroughObjectMapper name. This is achieved by adding a second reference to their
// MappedFieldType using the remaining suffix.
Map<String, PassThroughObjectMapper> passThroughFieldAliases = new HashMap<>();
for (PassThroughObjectMapper passThroughMapper : passThroughMappers) {
for (Mapper subfield : passThroughMapper.mappers.values()) {
if (subfield instanceof FieldMapper fieldMapper) {
String name = fieldMapper.simpleName();
// Check for conflict between PassThroughObjectMapper subfields.
PassThroughObjectMapper conflict = passThroughFieldAliases.put(name, passThroughMapper);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, you could also sort the passThroughMappers in ascending order and simply let mappers with a higher priority override i.e. put without checking for conflicts.

Your solution works just as well so feel free to ignore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If priority is going to be required, this does feel simpler to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to check for conflicts, to avoid cases with inconsistent behavior, depending on the order they show up in the FieldTypeLookup.

if (conflict != null) {
// Check the priorities of the conflicting objects.
if (conflict.priority() > passThroughMapper.priority()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's reverse the priority? A field from a passthrough field with a higher priority wins.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the case? If the inserted object has higher priority we leave it there, otherwise the conflicting one is put back. In both cases, fullNameToFieldType will contain the alias for the object with the highest priority?

passThroughFieldAliases.put(name, conflict);
continue;
}
} else if (fullNameToFieldType.containsKey(name)) {
// There's an existing field or alias for the same field.
continue;
}
MappedFieldType fieldType = fieldMapper.fieldType();
fullNameToFieldType.put(name, fieldType);
if (fieldType instanceof DynamicFieldType) {
dynamicFieldTypes.put(name, (DynamicFieldType) fieldType);
}
}
}
}

for (MappedFieldType fieldType : RuntimeField.collectFieldTypes(runtimeFields).values()) {
// this will override concrete fields with runtime fields that have the same name
fullNameToFieldType.put(fieldType.name(), fieldType);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ private CacheKey() {}
* A lookup representing an empty mapping. It can be used to look up fields, although it won't hold any, but it does not
* hold a valid {@link DocumentParser}, {@link IndexSettings} or {@link IndexAnalyzers}.
*/
public static final MappingLookup EMPTY = fromMappers(Mapping.EMPTY, List.of(), List.of(), List.of());
public static final MappingLookup EMPTY = fromMappers(Mapping.EMPTY, List.of(), List.of());

private final CacheKey cacheKey = new CacheKey();

Expand All @@ -67,35 +67,40 @@ public static MappingLookup fromMapping(Mapping mapping) {
List<ObjectMapper> newObjectMappers = new ArrayList<>();
List<FieldMapper> newFieldMappers = new ArrayList<>();
List<FieldAliasMapper> newFieldAliasMappers = new ArrayList<>();
List<PassThroughObjectMapper> newPassThroughMappers = new ArrayList<>();
for (MetadataFieldMapper metadataMapper : mapping.getSortedMetadataMappers()) {
if (metadataMapper != null) {
newFieldMappers.add(metadataMapper);
}
}
for (Mapper child : mapping.getRoot()) {
collect(child, newObjectMappers, newFieldMappers, newFieldAliasMappers);
collect(child, newObjectMappers, newFieldMappers, newFieldAliasMappers, newPassThroughMappers);
}
return new MappingLookup(mapping, newFieldMappers, newObjectMappers, newFieldAliasMappers);
return new MappingLookup(mapping, newFieldMappers, newObjectMappers, newFieldAliasMappers, newPassThroughMappers);
}

private static void collect(
Mapper mapper,
Collection<ObjectMapper> objectMappers,
Collection<FieldMapper> fieldMappers,
Collection<FieldAliasMapper> fieldAliasMappers
Collection<FieldAliasMapper> fieldAliasMappers,
Collection<PassThroughObjectMapper> passThroughMappers
) {
if (mapper instanceof ObjectMapper) {
objectMappers.add((ObjectMapper) mapper);
} else if (mapper instanceof FieldMapper) {
fieldMappers.add((FieldMapper) mapper);
} else if (mapper instanceof FieldAliasMapper) {
fieldAliasMappers.add((FieldAliasMapper) mapper);
if (mapper instanceof PassThroughObjectMapper passThroughObjectMapper) {
passThroughMappers.add(passThroughObjectMapper);
objectMappers.add(passThroughObjectMapper);
} else if (mapper instanceof ObjectMapper objectMapper) {
objectMappers.add(objectMapper);
} else if (mapper instanceof FieldMapper fieldMapper) {
fieldMappers.add(fieldMapper);
} else if (mapper instanceof FieldAliasMapper fieldAliasMapper) {
fieldAliasMappers.add(fieldAliasMapper);
} else {
throw new IllegalStateException("Unrecognized mapper type [" + mapper.getClass().getSimpleName() + "].");
}

for (Mapper child : mapper) {
collect(child, objectMappers, fieldMappers, fieldAliasMappers);
collect(child, objectMappers, fieldMappers, fieldAliasMappers, passThroughMappers);
}
}

Expand All @@ -111,22 +116,29 @@ private static void collect(
* @param mappers the field mappers
* @param objectMappers the object mappers
* @param aliasMappers the field alias mappers
* @param passThroughMappers the pass-through mappers
* @return the newly created lookup instance
*/
public static MappingLookup fromMappers(
Mapping mapping,
Collection<FieldMapper> mappers,
Collection<ObjectMapper> objectMappers,
Collection<FieldAliasMapper> aliasMappers
Collection<FieldAliasMapper> aliasMappers,
Collection<PassThroughObjectMapper> passThroughMappers
) {
return new MappingLookup(mapping, mappers, objectMappers, aliasMappers);
return new MappingLookup(mapping, mappers, objectMappers, aliasMappers, passThroughMappers);
}

public static MappingLookup fromMappers(Mapping mapping, Collection<FieldMapper> mappers, Collection<ObjectMapper> objectMappers) {
return new MappingLookup(mapping, mappers, objectMappers, List.of(), List.of());
}

private MappingLookup(
Mapping mapping,
Collection<FieldMapper> mappers,
Collection<ObjectMapper> objectMappers,
Collection<FieldAliasMapper> aliasMappers
Collection<FieldAliasMapper> aliasMappers,
Collection<PassThroughObjectMapper> passThroughMappers
) {
this.totalFieldsCount = mapping.getRoot().getTotalFieldsCount();
this.mapping = mapping;
Expand Down Expand Up @@ -172,13 +184,14 @@ private MappingLookup(
}
}

PassThroughObjectMapper.checkForInvalidPriorities(passThroughMappers);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is called when creating or updating an index template as it doesn't affect a concrete index, yet. In any case, I think it would be prudent to add a test case for an index template with two passthrough field types that don't specify a priority to see if this gets caught.

If this indeed turns out not to be the appropriate place for the validation, an alternative may be to validate this in the root object mapper. Yet another approach could be to allow duplicate priorities and then fall back to lexicographical ordering if the priority is the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So templates are validated in MetaIndexTemplateServervice#validateTemplate() line 1812.
The merge method at some point creates a new DocumentMapper instance and then the MappingLookup gets created.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for double-checking!

final Collection<RuntimeField> runtimeFields = mapping.getRoot().runtimeFields();
this.fieldTypeLookup = new FieldTypeLookup(mappers, aliasMappers, runtimeFields);
this.fieldTypeLookup = new FieldTypeLookup(mappers, aliasMappers, passThroughMappers, runtimeFields);
if (runtimeFields.isEmpty()) {
// without runtime fields this is the same as the field type lookup
this.indexTimeLookup = fieldTypeLookup;
} else {
this.indexTimeLookup = new FieldTypeLookup(mappers, aliasMappers, Collections.emptyList());
this.indexTimeLookup = new FieldTypeLookup(mappers, aliasMappers, passThroughMappers, Collections.emptyList());
}
// make all fields into compact+fast immutable maps
this.fieldMappers = Map.copyOf(fieldMappers);
Expand Down