Create aliases for PassThroughObjectMapper subfields in FieldTypeLookup #106829

kkrik-es · 2024-03-27T16:50:06Z

Move the logic for creating aliases from RootObjectMapper to FieldTypeLookup. We no longer create FieldAliasMapper fields but just the field names under the pass-through path to the same MappedFieldType - just as it happens for FieldAliasMapper objects. As a bonus, since aliases are created on-the-fly, it's now possible to handle conflicts gracefully using a superseded_by param in the PassThroughObjectMapper spec.

FieldTypeLookup is used in fieldcaps API so the aliases are expected to show up there, but not in the index mappings.

Related to #103648, #103567

elasticsearchmachine · 2024-04-02T10:56:12Z

Pinging @elastic/es-search (Team:Search)

elasticsearchmachine · 2024-04-02T10:56:13Z

Pinging @elastic/es-storage-engine (Team:StorageEngine)

martijnvg

I did an initial review round. I like the fact that this avoids updating the mapping with alias field mappers as part of using passthrough field mapper..

martijnvg · 2024-04-04T07:56:49Z

server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java

+        // PassThroughObjectMapper name. This is achieved by adding a second reference to their
+        // MappedFieldType using the remaining suffix.
+        Map<String, PassThroughObjectMapper> passThroughFieldAliases = new HashMap<>();
+        for (FieldMapper fieldMapper : fieldMappers) {


This can potentially be very expensive loop (depending on the number of fields and passthrough fields). Is there a less expensive way to do this?

This is achieved by adding a second reference to their MappedFieldType using the remaining suffix

Can you share a mapping that would look like this? I'm trying to understand this better.

Right, switched to just check the passthrough subfields.

This is achieved by adding a second reference to their MappedFieldType using the remaining suffix

This is the alias that's added to the lookup, just like through the FieldAliasMappers before.

server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java

martijnvg · 2024-04-04T09:51:51Z

server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java

    PassThroughObjectMapper(
        String name,
        String fullPath,
        Explicit<Boolean> enabled,
        Dynamic dynamic,
        Map<String, Mapper> mappers,
-        Explicit<Boolean> timeSeriesDimensionSubFields
+        Explicit<Boolean> timeSeriesDimensionSubFields,
+        Set<String> supersededBy


Maybe an alternative way to deal with conflicts could be having a priority mapping attribute instead? (range from: 0 to max integer)

If there are multiple passthrough fields then a priority attribute is required on all passthrough fields. If there're passthough fields with the same priority we fail with a validation error. If multiple passthrough fields match we pick the passthough field with the highest priority.

I think this could simplify to validation logic / logic in FieldTypeLookup?
Note that index templates have a similar notion of priority.

The problem with this is that a user may start with one passthrough field, then add another one, then notice that there are conflicts. I don't think we should require users to have a finalized list of pass-through objects from the get go, @felixbarny too.

The problem with this is that a user may start with one passthrough field, then add another one, then notice that there are conflicts.

If the second passthrough field is added, the new passthrough field will need a priority attribute and the existing passthrough field as well. I don't think that this is an issue? Passthrough fields are defined on templates, and template validation will fail when the priorities are missing or misaligned.

Without having thought very deeply about this, I was also thinking of a numeric priority mapping parameter. But need to read more to get a better understanding of what supersededBy is doing.

For OTel mappings, the precedence (from highest to lowest) would look something like this:

Top-level/non passthrough fields

resource.attributes.*

scope.attributes.*

attributes.*

To me, it seems that could be achieved with a simple numeric priority mapping parameter, or, at least with a single value for supersededBy.

Switched to using priorities. An issue that I see is what happens with existing pass-through objects missing priorities.. I can check for the existence of aliases and bypass the check for priorities, but that's not great either.. Thoughts?

I think we haven't really used the passthrough field type outside of testing, so this breaking change is probably fine. Alternatively, we could not require passthrough fields to have unique priorities and fall back to lexicographical ordering if the priority is the same.

Let's make priority required. This field type is undocumented and not ga-ed. This field type is only used for cross team testing at the moment.

felixbarny · 2024-04-05T07:06:11Z

server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java

+                if (subfield instanceof FieldMapper fieldMapper) {
+                    String name = fieldMapper.simpleName();
+                    // Check for conflict between PassThroughObjectMapper subfields.
+                    PassThroughObjectMapper conflict = passThroughFieldAliases.put(name, passThroughMapper);


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.

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

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.

felixbarny · 2024-04-05T07:09:27Z

server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java

@@ -172,13 +184,14 @@ private MappingLookup(
            }
        }

+        PassThroughObjectMapper.checkForInvalidPriorities(passThroughMappers);


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.

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.

Nice, thanks for double-checking!

martijnvg

This is getting close, let's make priority attribute required?

martijnvg · 2024-04-05T09:20:25Z

server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java


    public static class Builder extends ObjectMapper.Builder {

        // Controls whether subfields are configured as time-series dimensions.
        protected Explicit<Boolean> timeSeriesDimensionSubFields = Explicit.IMPLICIT_FALSE;

+        // Controls which pass-through fields take precedence in case of conflicting aliases.
+        protected int priority = 0;


Let's make this attribute required.

Just to put another idea out there. We could define that the default is 0, basically this is already what it does now. When adding another passthrough field, you don't need to update the existing one if the new field has a higher priority. Not defining the priority on the second field would fail because of the validation that the priority needs to be unique.

But I'd also be fine with making the priority required, even if there's only one passthrough field in the mappings.

I think it's cleaner to just make it required.

martijnvg · 2024-04-05T09:26:00Z

server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java

+                if (subfield instanceof FieldMapper fieldMapper) {
+                    String name = fieldMapper.simpleName();
+                    // Check for conflict between PassThroughObjectMapper subfields.
+                    PassThroughObjectMapper conflict = passThroughFieldAliases.put(name, passThroughMapper);


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

felixbarny · 2024-04-05T13:46:23Z

server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java

+ *
+ * In case different pass-through objects contain subfields with the same name (excluding the pass-through prefix), their aliases conflict.
+ * To resolve this, the pass-through spec specifies which object takes precedence through required parameter "priority"; non-negative
+ * integer values are accepted, with the highest priority value winning in case of conflicting aliases.


Priority 1 is the highest priority, right?

No 0 is the lowest. I can change it if you feel strongly, do we have another case where we use priority in a field?

Oh, I see. No strong opinion here, mainly wanted to understand whether priority is ascending or descending. Maybe a slight preference for only allowing positive integers (excluding zero).

do we have another case where we use priority in a field?

I've found
https://github.com/elastic/elasticsearch/blob/main/server/src/main/java/org/elasticsearch/common/Priority.java. Interesting it does have a priority 0 (IMMEDIATE). But that seems to be a bit of a different use case.

I've also found we're using priority for index templates and the higher the number, the higher the priority, so priority 1 would be a very low priority.

Right, I think Martijn suggested to use the same approach as the last example.

martijnvg

Left two comments. LGTM otherwise.

martijnvg · 2024-04-08T11:48:40Z

server/src/main/java/org/elasticsearch/index/mapper/PassThroughObjectMapper.java

 import java.util.Locale;
 import java.util.Map;

 import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeBooleanValue;
+import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeIntegerValue;

 /**
 * Mapper for pass-through objects.


Maybe add a disclaimer that this is a field type that is experimental, undocumented and shouldn't be used in production and that it exists today for prototyping purposes only.

martijnvg · 2024-04-08T12:00:25Z

server/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java

+                    PassThroughObjectMapper conflict = passThroughFieldAliases.put(name, passThroughMapper);
+                    if (conflict != null) {
+                        // Check the priorities of the conflicting objects.
+                        if (conflict.priority() > passThroughMapper.priority()) {


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

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?

kkrik-es · 2024-04-09T09:16:05Z

@elasticsearchmachine run elasticsearch-ci/packaging-tests-unix-sample

kkrik-es · 2024-04-09T10:00:10Z

@elasticsearchmachine run elasticsearch-ci/bwc-snapshots

# Conflicts: # modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml # server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java

kkrik-es · 2024-04-25T12:00:49Z

Gentle ping for @javanna, ptal.

# Conflicts: # server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java

# Conflicts: # server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java

kkrik-es · 2024-05-17T08:48:49Z

@javanna I'm submitting this to get some mileage with testing. Please let me know if you have comments, I'm happy to address them in a follow-up.

javanna · 2024-05-17T13:28:46Z

I just reviewed this and I am happy with the changes made, I think it's a net improvement. Thanks for taking the time!

kkrik-es · 2024-05-17T13:46:20Z

Great to hear Luca, thanks for the guidance here!

Alias PassThroughObjectMapper subfields in FieldTypeLookup

009ee70

kkrik-es added >non-issue :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team Team:StorageEngine :StorageEngine/Mapping The storage related side of mappings labels Mar 27, 2024

kkrik-es self-assigned this Mar 27, 2024

elasticsearchmachine added the v8.14.0 label Mar 27, 2024

kkrik-es added 6 commits March 27, 2024 19:05

small refactor

c54fd29

conflict resolution

6ec8699

add check for circular deps

05a6016

Merge branch 'main' into fix/passthrough-no-alias

cbd6076

use lexicographical ordering for conflicting aliases

c084c31

use lexicographical ordering for conflicting aliases

5109d56

kkrik-es marked this pull request as ready for review April 2, 2024 10:55

kkrik-es requested a review from martijnvg April 2, 2024 10:55

martijnvg reviewed Apr 4, 2024

View reviewed changes

kkrik-es added 3 commits April 4, 2024 15:15

Merge branch 'main' into fix/passthrough-no-alias

145ba0d

simplify loop producing aliases

d867e30

use priorities for passthrough deduplication

0f9b08d

felixbarny reviewed Apr 5, 2024

View reviewed changes

felixbarny approved these changes Apr 5, 2024

View reviewed changes

martijnvg reviewed Apr 5, 2024

View reviewed changes

make priority param required

7f2dc2a

felixbarny reviewed Apr 5, 2024

View reviewed changes

kkrik-es requested a review from javanna April 8, 2024 07:49

martijnvg approved these changes Apr 8, 2024

View reviewed changes

add passthrough disclaimer

45aa214

kkrik-es added 4 commits April 8, 2024 15:20

small refactor

3cd0f1c

small refactor

ba3aecc

revert refactor

a72f16d

Merge branch 'refs/heads/main' into fix/passthrough-no-alias

201b882

Merge branch 'refs/heads/main' into fix/passthrough-no-alias

f700d59

Merge branch 'refs/heads/main' into fix/passthrough-no-alias

8553b99

elasticsearchmachine added v8.15.0 and removed v8.14.0 labels Apr 17, 2024

Merge branch 'refs/heads/main' into fix/passthrough-no-alias

e1241c5

# Conflicts: # modules/data-streams/src/yamlRestTest/resources/rest-api-spec/test/data_stream/150_tsdb.yml # server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java

kkrik-es added 4 commits April 29, 2024 14:27

Merge branch 'refs/heads/main' into fix/passthrough-no-alias

879afde

node feature for pass-through priotity

20ed13d

Merge branch 'refs/heads/main' into fix/passthrough-no-alias

1a5823c

# Conflicts: # server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java

Merge branch 'refs/heads/main' into fix/passthrough-no-alias

f7e78aa

# Conflicts: # server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java

kkrik-es merged commit 25e3551 into elastic:main May 17, 2024
15 checks passed

kkrik-es deleted the fix/passthrough-no-alias branch May 17, 2024 10:23

felixbarny mentioned this pull request Jul 1, 2024

Fix assertion error when using passthrough with contradicting fields #110326

Merged

Create aliases for PassThroughObjectMapper subfields in FieldTypeLookup #106829

Create aliases for PassThroughObjectMapper subfields in FieldTypeLookup #106829

Conversation

kkrik-es commented Mar 27, 2024 • edited Loading

elasticsearchmachine commented Apr 2, 2024

elasticsearchmachine commented Apr 2, 2024

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkrik-es commented Apr 9, 2024

kkrik-es commented Apr 9, 2024

kkrik-es commented Apr 25, 2024

kkrik-es commented May 17, 2024

javanna commented May 17, 2024

kkrik-es commented May 17, 2024

kkrik-es commented Mar 27, 2024 •

edited

Loading