Correctly identify parent of copy_to destination field for synthetic source purposes #113153

lkts · 2024-09-18T23:22:19Z

This PR is a continuation of #112294 and handles a case of copy_to destination fields being inside object that was not properly addressed there.

…source purposes

lkts · 2024-09-19T18:36:14Z

I am not sure what 9.0.0 bwc snapshots are?

elasticsearchmachine · 2024-09-19T22:17:05Z

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

kkrik-es · 2024-09-20T06:36:34Z

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

+        if (indexSettings.getSkipIgnoredSourceWrite() == false) {
+            /*
+            Mark this field as containing copied data meaning it should not be present
+            in synthetic _source (to be consistent with stored _source).


Nit: update text, no given field here..

kkrik-es · 2024-09-20T06:37:06Z

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

+            Mark this field as containing copied data meaning it should not be present
+            in synthetic _source (to be consistent with stored _source).
+            Ignored source values take precedence over standard synthetic source implementation
+            so by adding this nothing entry we "disable" field in synthetic source.


Nit: s/nothing/void/

kkrik-es · 2024-09-20T06:38:51Z

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

+                if (parent == null) {
+                    // There are scenarios when this can happen:
+                    // 1. all values of the field that is the source of copy_to are null
+                    // 2. copy_to points at a field inside disabled object


Nit: s/inside disabled/inside a disabled/

kkrik-es · 2024-09-20T06:41:13Z

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

+                }
+                int offset = parent.isRoot() ? 0 : parent.fullPath().length() + 1;
+                ignoredFieldValues.add(
+                    new IgnoredSourceFieldMapper.NameValue(copyToField, offset, XContentDataHelper.nothing(), context.doc())


Rename nothing() to voidValue()?

kkrik-es · 2024-09-20T06:44:37Z

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

+            }
+
+            // Go one level down if possible
+            var pathComponents = pathInCurrent.split("\\.");


Nit: do this once before the loop, increment the start element on each iteration?

kkrik-es · 2024-09-20T06:47:49Z

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

        private List<IgnoredSourceFieldMapper.NameValue> ignoredValues;
+        // If this loader has anything to write.
+        // In special cases this can be false even if doc values loaders or stored field loaders
+        // have values.


Nit: reference the copy_to case as an example?

kkrik-es · 2024-09-20T06:50:12Z

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

            }

-            if (ignoredValues != null && ignoredValues.isEmpty() == false) {
-                // Use an ordered map between field names and writer functions, to order writing by field name.


Copy this comment.

kkrik-es · 2024-09-20T06:53:11Z

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

        }

+        @Override
+        public void prepare() {


I kinda like this version, compared to running everything in write().

kkrik-es · 2024-09-20T06:55:03Z

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

        DocValuesLoader docValuesLoader(LeafReader leafReader, int[] docIdsInLeaf) throws IOException;

+        /**
+         Perform any preprocessing needed before producing synthetic source.


Maybe add: " to deduce whether this mapper (and its children, if any) have values to write"

kkrik-es · 2024-09-20T06:56:21Z

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

+         The expectation is for this method to be called before {@link SyntheticFieldLoader#hasValue()}
+         and {@link SyntheticFieldLoader#write(XContentBuilder)} are used.
+         */
+        default void prepare() {


prepareHasValue? to make it more concrete.

I like this more because it's a verb and it does not have has in there which has boolean associations.

kkrik-es · 2024-09-20T07:04:29Z

...pi-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.create/20_synthetic_source.yml

+        k: "hey"
+  - match:
+      hits.hits.0.fields:
+        a.b.c: [ "hey" ]


Why do we decide to match a.b.c versus a.b\.c'? I was wondering about that in findParentObject`..

I think I got it, we first check for a leaf field match before checking for object matches.

kkrik-es · 2024-09-20T07:15:54Z

This leads to a memory bump as we first create the map containing all values to write per doc and then start writing. I'd think this is not that huge per doc (a few MB, worst case), so it should be acceptable.

The logic is fairly clean, well done.

kkrik-es · 2024-09-20T07:18:32Z

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

+            return;
+        }
+
        List<NameValue> ignoredFieldValues = new ArrayList<>(context.getIgnoredFieldValues());


Nit: wrap in ArrayList within the branch below, otherwise we don't mutate the returned list.

But we use this list outside of the branch?

Ah i see what you mean.

salvatore-campagna · 2024-09-20T15:25:30Z

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

+
+            // Go one level down if possible
+            var pathComponents = pathInCurrent.split("\\.");
+            var childMapperName = new StringBuilder();


Create the StringBuilder outside the loop and then call setLength(0) in the loop to avoid creating it multiple times?

I looked at setLength and it replaces internal byte array with a new one so i think it's equivalent?

salvatore-campagna · 2024-09-20T15:31:27Z

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

+        String pathInCurrent = leafFieldPath;
+
+        while (current != null) {
+            if (current.mappers.containsKey(pathInCurrent)) {


I am wondering if we have to check for pathInCurrent to be null since we pass it to containsKey. leafFieldPath is copyToFields right?

I can't think of a scenario when we would add null as a copy to field in document parser context. I am not sure what this code should do in that case too.

lkts · 2024-09-20T15:47:35Z

@elasticmachine update branch

lkts · 2024-09-20T16:44:45Z

@elasticmachine update branch

lkts · 2024-09-20T18:39:18Z

Test failures look like preexisting issues, there are open issues for them like #113301. I guess they are muted on main but not in 8.16 branch?

lkts · 2024-09-20T19:08:08Z

I'll go ahead and merge this so that we have data generation tests running.

elasticsearchmachine · 2024-09-20T19:10:12Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 113153

…source purposes (elastic#113153) (cherry picked from commit b9855b8) # Conflicts: # server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java

lkts · 2024-09-20T19:21:43Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation

…hetic source purposes (#113153) (#113307) * Correctly identify parent of copy_to destination field for synthetic source purposes (#113153) (cherry picked from commit b9855b8) # Conflicts: # server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java * Comma

lkts added >non-issue auto-backport-and-merge :StorageEngine/Logs You know, for Logs v8.16.0 labels Sep 18, 2024

elasticsearchmachine added the v9.0.0 label Sep 18, 2024

lkts force-pushed the fix_synthetic_source_copy_to branch from 2a0a8e1 to f61215c Compare September 18, 2024 23:23

lkts added 2 commits September 19, 2024 09:02

Correctly identify parent of copy_to destination field for synthetic …

4035121

…source purposes

fix tests

9ad3fba

lkts force-pushed the fix_synthetic_source_copy_to branch from fc3da97 to 9ad3fba Compare September 19, 2024 16:14

Small cleanup

270f5a9

lkts marked this pull request as ready for review September 19, 2024 22:16

lkts requested review from kkrik-es, martijnvg and salvatore-campagna September 19, 2024 22:16

elasticsearchmachine added the Team:StorageEngine label Sep 19, 2024

cleanup

5fef1c2

kkrik-es reviewed Sep 20, 2024

View reviewed changes

kkrik-es approved these changes Sep 20, 2024

View reviewed changes

kkrik-es reviewed Sep 20, 2024

View reviewed changes

salvatore-campagna reviewed Sep 20, 2024

View reviewed changes

elasticmachine and others added 2 commits September 20, 2024 17:47

Merge branch 'main' into fix_synthetic_source_copy_to

1650afc

Address feedback

baeda6e

elasticmachine and others added 2 commits September 20, 2024 18:44

Merge branch 'main' into fix_synthetic_source_copy_to

e6deced

style

5ab4c31

lkts merged commit b9855b8 into elastic:main Sep 20, 2024
12 of 17 checks passed

lkts deleted the fix_synthetic_source_copy_to branch September 20, 2024 19:08

elasticsearchmachine added the backport pending label Sep 20, 2024

lkts mentioned this pull request Sep 20, 2024

[8.x] Correctly identify parent of copy_to destination field for synthetic source purposes (#113153) #113307

Merged

Correctly identify parent of copy_to destination field for synthetic source purposes #113153

Correctly identify parent of copy_to destination field for synthetic source purposes #113153

Uh oh!

Conversation

lkts commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lkts commented Sep 19, 2024

Uh oh!

elasticsearchmachine commented Sep 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkrik-es Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkts Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkrik-es commented Sep 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkts commented Sep 20, 2024

Uh oh!

lkts commented Sep 20, 2024

Uh oh!

lkts commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lkts commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented Sep 20, 2024

💔 Backport failed

Uh oh!

lkts commented Sep 20, 2024

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

lkts commented Sep 18, 2024 •

edited

Loading

kkrik-es Sep 20, 2024 •

edited

Loading

lkts Sep 20, 2024 •

edited

Loading

lkts commented Sep 20, 2024 •

edited

Loading

lkts commented Sep 20, 2024 •

edited

Loading