Add `unmatch_mapping_type`, and support array of types #103171

axw · 2023-12-08T04:01:43Z

Add an unmatch_mapping_type condition to dynamic templates (supporting one or more types), and add support for specifying a list of types to match_mapping_type.

Closes #102795
Closes #102807

Add an unmatch_mapping_type condition to dynamic templates, and add support for specifying a list of types to match_mapping_type.

axw · 2023-12-08T04:05:53Z

@felixbarny @javanna I took a stab at addressing the linked issues. Could you please tell me if this looks directionally sensible, and if you can spot any major issues? If it looks alright I'll proceed to add docs and probably expand the tests a bit.

The main thing I'm unsure about is backwards compatibility with respect to the serialisation changes. Do we need to do something special for cluster upgrades?

felixbarny · 2023-12-08T08:27:53Z

A good template for this change would be this PR:

Allow multiple field names/patterns for (path_)(un)match (#66364) #95558

The main thing I'm unsure about is backwards compatibility with respect to the serialisation changes. Do we need to do something special for cluster upgrades?

Seems like that PR doesn't do anything special here with regards to the serialization changes. However, I think there may be a problematic situation that can occur in this code line in a mixed version cluster scenario (such as when doing a rolling upgrade):

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

Line 314 in b9c2980

    
           final XContentFieldType xContentFieldType = XContentFieldType.fromString(matchMappingType);

If one node in a cluster gets updated and the user sends a mapping update request to that node, it may propagate that mapping to older nodes that expect the content of match_mapping_type to be a string and they'd fail when an array of values is passed.

But maybe all of that is a non issue, depending on how the update process works. IIRC, at first, all non-master nodes are updated before the master node. Also, updating the mapping would go though the master node that triggers a cluster state update to propagate the mappings to other nodes. During that process, the master node would validate the mappings. I think that makes sure that ES will only accept mappings with an array in match_mapping_type when all nodes in the cluster are updated.

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

…plate.java Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>

- Use addEntriesToPatternList like match/path_match etc. - Serialise (un)match_mapping_type as specified, except for single-value lists which are always serialised as a non-list value. Implicit "match_mapping_type: *" is still not serialised, but an explicit setting will now be serialised where it was not before.

... to highlight the original purpose, which is to unmatch an internal object field whose path may match a leaf field.

axw · 2023-12-11T04:04:02Z

@elasticmachine run elasticsearch-ci/packaging-tests-windows-sample

elasticsearchmachine · 2023-12-11T07:28:09Z

Hi @axw, I've created a changelog YAML for you.

felixbarny

Just a minor suggestion, otherwise LGTM. This also needs to be reviewed by the search team before merging.

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

Move runtime-supported type filtering.

elasticsearchmachine · 2023-12-12T08:00:21Z

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

axw · 2024-01-25T08:12:34Z

@salvatore-campagna would you be able to review this PR? Or otherwise tag someone else more appropriate? 🙏

elasticsearchmachine · 2024-01-25T08:47:16Z

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

salvatore-campagna · 2024-02-09T14:04:37Z

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

@@ -257,7 +260,8 @@ static DynamicTemplate parse(String name, Map<String, Object> conf) throws Mappe
        List<String> pathUnmatch = new ArrayList<>(4);
        Map<String, Object> mapping = null;
        boolean runtime = false;
-        String matchMappingType = null;
+        List<String> matchMappingType = new ArrayList<>(4);


Why is the size of the array 4? Just to have a size that is lower than he default?

This uses the same initial size of the array list as the other lists a few lines above. I think this is because we expect most of these to have one or a few values, so a custom default value 4 aims for more efficiency than the generic default value.

salvatore-campagna · 2024-02-09T14:12:13Z

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

+                builder.field("unmatch_mapping_type", unmatchMappingType.get(0));
+            } else {
+                builder.field("unmatch_mapping_type", unmatchMappingType);
+            }


Logic seems similar...maybe extract a method just passing a list and a string?

let me add that

salvatore-campagna · 2024-02-09T14:16:49Z

LGTM, I left a question and a comment.

…tch-mapping-type

… size of the list

Add an `unmatch_mapping_type` condition to dynamic templates (supporting one or more types), and add support for specifying a list of types to `match_mapping_type`. Closes elastic#102795 Closes elastic#102807

Add unmatch_mapping_type; support array of types

7d2a5a4

Add an unmatch_mapping_type condition to dynamic templates, and add support for specifying a list of types to match_mapping_type.

elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v8.13.0 labels Dec 8, 2023

felixbarny reviewed Dec 8, 2023

View reviewed changes

felixbarny mentioned this pull request Dec 8, 2023

x-pack/plugin/core: add match_mapping_type to ecs@mappings dynamic templates #103035

Merged

axw and others added 5 commits December 9, 2023 08:23

Update server/src/main/java/org/elasticsearch/index/mapper/DynamicTem…

e50dcf3

…plate.java Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>

Fix test

ccfda06

Add YAML REST test

b6c8134

Add another unit test for unmatch_mapping_type

952d816

... to highlight the original purpose, which is to unmatch an internal object field whose path may match a leaf field.

felixbarny added >feature :Search/Mapping Index mappings, including merging and defining field types labels Dec 11, 2023

Update docs/changelog/103171.yaml

10276ed

felixbarny approved these changes Dec 11, 2023

View reviewed changes

server/src/main/java/org/elasticsearch/index/mapper/DynamicTemplate.java Outdated Show resolved Hide resolved

axw added 2 commits December 12, 2023 13:44

Address review comment

78a0007

Move runtime-supported type filtering.

Update docs

cfdf7e8

axw marked this pull request as ready for review December 12, 2023 07:59

elasticsearchmachine added the Team:Search Meta label for search team label Dec 12, 2023

Fix docs

8aef59f

felixbarny requested a review from salvatore-campagna December 19, 2023 14:34

ruflin assigned axw Dec 20, 2023

siposea added the :StorageEngine/Logs You know, for Logs label Jan 25, 2024

elasticsearchmachine added the Team:StorageEngine label Jan 25, 2024

salvatore-campagna approved these changes Feb 9, 2024

View reviewed changes

felixbarny added 2 commits February 9, 2024 15:35

Merge remote-tracking branch 'origin/main' into dynamic-template-unma…

0f9336a

…tch-mapping-type

Extract method to serialize a string or array field, depending on the…

2592cbe

… size of the list

felixbarny added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 9, 2024

elasticsearchmachine merged commit 5f90978 into elastic:main Feb 9, 2024
15 checks passed

axw deleted the dynamic-template-unmatch-mapping-type branch February 9, 2024 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `unmatch_mapping_type`, and support array of types #103171

Add `unmatch_mapping_type`, and support array of types #103171

axw commented Dec 8, 2023

axw commented Dec 8, 2023

felixbarny commented Dec 8, 2023

axw commented Dec 11, 2023

elasticsearchmachine commented Dec 11, 2023

felixbarny left a comment

elasticsearchmachine commented Dec 12, 2023

axw commented Jan 25, 2024

elasticsearchmachine commented Jan 25, 2024

salvatore-campagna Feb 9, 2024

felixbarny Feb 9, 2024

salvatore-campagna Feb 9, 2024

felixbarny Feb 9, 2024

salvatore-campagna commented Feb 9, 2024

Add unmatch_mapping_type, and support array of types #103171

Add unmatch_mapping_type, and support array of types #103171

Conversation

axw commented Dec 8, 2023

axw commented Dec 8, 2023

felixbarny commented Dec 8, 2023

axw commented Dec 11, 2023

elasticsearchmachine commented Dec 11, 2023

felixbarny left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Dec 12, 2023

axw commented Jan 25, 2024

elasticsearchmachine commented Jan 25, 2024

salvatore-campagna Feb 9, 2024

Choose a reason for hiding this comment

felixbarny Feb 9, 2024

Choose a reason for hiding this comment

salvatore-campagna Feb 9, 2024

Choose a reason for hiding this comment

felixbarny Feb 9, 2024

Choose a reason for hiding this comment

salvatore-campagna commented Feb 9, 2024

Add `unmatch_mapping_type`, and support array of types #103171

Add `unmatch_mapping_type`, and support array of types #103171