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 all 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 @@ -70,6 +72,7 @@ public class TSDBPassthroughIndexingIT extends ESSingleNodeTestCase {
},
"attributes": {
"type": "passthrough",
"priority": 0,
"dynamic": true,
"time_series_dimension": true
},
Expand Down Expand Up @@ -197,31 +200,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 @@ -240,7 +240,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 @@ -631,10 +631,12 @@ public void testGenerateRoutingPathFromPassThroughObject() throws Exception {
"properties": {
"labels": {
"type": "passthrough",
"time_series_dimension": true
"time_series_dimension": true,
"priority": 2
},
"metrics": {
"type": "passthrough"
"type": "passthrough",
"priority": 1
},
"another_field": {
"type": "keyword"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,8 @@ index without timestamp with pipeline:
---
dynamic templates:
- requires:
cluster_features: ["gte_v8.13.0"]
reason: "Support for dynamic fields was added in 8.13"
cluster_features: ["mapper.pass_through_priority"]
reason: support for priority in passthrough objects
- do:
allowed_warnings:
- "index template [my-dynamic-template] has index patterns [k9s*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [my-dynamic-template] will take precedence during new index creation"
Expand All @@ -219,6 +219,7 @@ dynamic templates:
type: passthrough
dynamic: true
time_series_dimension: true
priority: 0
dynamic_templates:
- counter_metric:
mapping:
Expand Down Expand Up @@ -326,8 +327,8 @@ dynamic templates:
---
dynamic templates - conflicting aliases:
- requires:
cluster_features: ["gte_v8.13.0"]
reason: "Support for dynamic fields was added in 8.13"
cluster_features: ["mapper.pass_through_priority"]
reason: support for priority in passthrough objects
- do:
allowed_warnings:
- "index template [my-dynamic-template] has index patterns [k9s*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [my-dynamic-template] will take precedence during new index creation"
Expand All @@ -350,10 +351,12 @@ dynamic templates - conflicting aliases:
type: passthrough
dynamic: true
time_series_dimension: true
priority: 2
resource_attributes:
type: passthrough
dynamic: true
time_series_dimension: true
priority: 1
dynamic_templates:
- counter_metric:
mapping:
Expand Down Expand Up @@ -391,7 +394,7 @@ dynamic templates - conflicting aliases:
filterA:
filter:
term:
dim: "C"
dim: A
aggs:
tsids:
terms:
Expand All @@ -410,7 +413,7 @@ dynamic templates - conflicting aliases:
filterA:
filter:
term:
attributes.dim: A
resource_attributes.dim: C
aggs:
tsids:
terms:
Expand All @@ -423,8 +426,8 @@ dynamic templates - conflicting aliases:
---
dynamic templates with nesting:
- requires:
cluster_features: ["gte_v8.13.0"]
reason: "Support for dynamic fields was added in 8.13"
cluster_features: ["mapper.pass_through_priority"]
reason: support for priority in passthrough objects
- do:
allowed_warnings:
- "index template [my-dynamic-template] has index patterns [k9s*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [my-dynamic-template] will take precedence during new index creation"
Expand All @@ -447,13 +450,15 @@ dynamic templates with nesting:
type: passthrough
dynamic: true
time_series_dimension: true
priority: 2
resource:
type: object
properties:
attributes:
type: passthrough
dynamic: true
time_series_dimension: true
priority: 1
dynamic_templates:
- counter_metric:
mapping:
Expand Down Expand Up @@ -580,8 +585,9 @@ dynamic templates with nesting:
---
dynamic templates with incremental indexing:
- requires:
cluster_features: ["gte_v8.13.0"]
reason: "Support for dynamic fields was added in 8.13"
cluster_features: ["mapper.pass_through_priority"]
reason: support for priority in passthrough objects

- do:
allowed_warnings:
- "index template [my-dynamic-template] has index patterns [k9s*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [my-dynamic-template] will take precedence during new index creation"
Expand All @@ -604,13 +610,15 @@ dynamic templates with incremental indexing:
type: passthrough
dynamic: true
time_series_dimension: true
priority: 2
resource:
type: object
properties:
attributes:
type: passthrough
dynamic: true
time_series_dimension: true
priority: 1
dynamic_templates:
- counter_metric:
mapping:
Expand Down Expand Up @@ -774,8 +782,8 @@ dynamic templates with incremental indexing:
---
subobject in passthrough object auto flatten:
- requires:
cluster_features: ["gte_v8.13.0"]
reason: "Support for passthrough fields was added in 8.13"
cluster_features: ["mapper.pass_through_priority"]
reason: support for priority in passthrough objects
- do:
allowed_warnings:
- "index template [my-passthrough-template] has index patterns [k9s*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [my-passthrough-template] will take precedence during new index creation"
Expand All @@ -794,6 +802,7 @@ subobject in passthrough object auto flatten:
attributes:
type: passthrough
time_series_dimension: true
priority: 0
properties:
subcategory:
type: object
Expand Down Expand Up @@ -833,11 +842,36 @@ 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 duplicate priority:
- requires:
cluster_features: ["mapper.pass_through_priority"]
reason: support for priority in passthrough objects
- do:
catch: /has a conflicting param/
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
priority: 1
resource.attributes:
type: passthrough
priority: 1
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) {
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?

// Keep the conflicting field if it has higher 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 @@ -19,6 +19,10 @@
public class MapperFeatures implements FeatureSpecification {
@Override
public Set<NodeFeature> getFeatures() {
return Set.of(IgnoredSourceFieldMapper.TRACK_IGNORED_SOURCE, RangeFieldMapper.NULL_VALUES_OFF_BY_ONE_FIX);
return Set.of(
IgnoredSourceFieldMapper.TRACK_IGNORED_SOURCE,
PassThroughObjectMapper.PASS_THROUGH_PRIORITY,
RangeFieldMapper.NULL_VALUES_OFF_BY_ONE_FIX
);
}
}