Add option to skip using _ignored_source field for synthetic source #112963

lkts · 2024-09-16T20:31:30Z

elasticsearchmachine · 2024-09-16T20:31:54Z

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

lkts · 2024-09-17T00:02:08Z

Diff is mostly tests.

kkrik-es · 2024-09-17T06:00:34Z

...src/test/java/org/elasticsearch/index/mapper/IgnoredSourceFieldMapperConfigurationTests.java

+
+        String syntheticSource = syntheticSource(mapperService, inputDocument);
+        // Values are not loaded.
+        assertEquals("{}", syntheticSource);


Flip to false, verify that the field gets read.

kkrik-es · 2024-09-17T06:01:15Z

...src/test/java/org/elasticsearch/index/mapper/IgnoredSourceFieldMapperConfigurationTests.java

+
+        String syntheticSource = syntheticSource(mapperService, inputDocument);
+        // Values are not loaded.
+        assertEquals("{}", syntheticSource);


kkrik-es · 2024-09-17T06:07:03Z

test/framework/src/main/java/org/elasticsearch/index/mapper/MapperServiceTestCase.java

                assertThat(roundTripSyntheticSource, equalTo(syntheticSource));
                validateRoundTripReader(syntheticSource, reader, roundTripReader);
            }
        }
    }

-    protected static String syntheticSource(DocumentMapper mapper, IndexReader reader, int docId) throws IOException {


I'd keep this flavor and pass empty indexSettings for the cases we don't care. That should reduce the churn for testing files..

martijnvg · 2024-09-17T07:06:31Z

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


    static final NodeFeature TRACK_IGNORED_SOURCE = new NodeFeature("mapper.track_ignored_source");

+    public static final Setting<Boolean> SKIP_IGNORED_SOURCE_WRITE_SETTING = Setting.boolSetting(


maybe add java doc on top of these public settings to explain what they do exactly?

the java doc is now empty

Yes, fixed.

martijnvg · 2024-09-17T07:14:51Z

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

            this.syntheticFieldLoaderLeafSupplier = fieldLoaderSupplier;
            this.requiredStoredFields = syntheticFieldLoaderLeafSupplier.get()
                .storedFieldLoaders()
                .map(Map.Entry::getKey)
                .collect(Collectors.toSet());
-            this.requiredStoredFields.add(IgnoredSourceFieldMapper.NAME);
+            if (indexSettings.getSkipIgnoredSourceRead() == false) {


My understanding was that we only need to ignore _ignore_source at read time, like is done here. This to avoid the potential errors when synthesizing the source. An escape hatch. Do we need ignore _ignore_source at write time?

I think it's better to add both, in case we have a bug at some point (...) with indexing. These settings won't be advertised, but we can give them to support in case we have a really urgent SDH (ingestion blocked etc).

Ok, I see. An another escape hatch if for some reason we have a bug at index time.

lkts · 2024-09-17T15:27:08Z

@elasticmachine update branch

martijnvg

LGTM - this approach is cleaner

martijnvg · 2024-09-17T18:41:06Z

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


    static final NodeFeature TRACK_IGNORED_SOURCE = new NodeFeature("mapper.track_ignored_source");

+    public static final Setting<Boolean> SKIP_IGNORED_SOURCE_WRITE_SETTING = Setting.boolSetting(


the java doc is now empty

…lastic#112963)

elasticsearchmachine · 2024-09-17T20:49:19Z

💔 Backport failed

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

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

lkts · 2024-09-17T21:41:02Z

💚 All backports created successfully

Status	Branch	Result
✅	8.15

Questions ?

Please refer to the Backport tool documentation

…lastic#112963) (cherry picked from commit 36b3549) # Conflicts: # server/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java # server/src/main/java/org/elasticsearch/index/IndexSettings.java # server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java # server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java

…112963) (#113065) (cherry picked from commit 36b3549) # Conflicts: # server/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java # server/src/main/java/org/elasticsearch/index/IndexSettings.java # server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java # server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java

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

…lastic#112963)

…112963) (#113057) Co-authored-by: Elastic Machine <[email protected]>

Add option to skip using _ignored_source field for synthetic source

8ea0aaa

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

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

elasticsearchmachine added v9.0.0 Team:StorageEngine labels Sep 16, 2024

lkts added 3 commits September 16, 2024 14:02

iter

cdffb59

Change everything

1ec39f3

fix

bb75a55

kkrik-es reviewed Sep 17, 2024

View reviewed changes

kkrik-es approved these changes Sep 17, 2024

View reviewed changes

martijnvg reviewed Sep 17, 2024

View reviewed changes

elasticmachine and others added 3 commits September 17, 2024 17:27

Merge branch 'main' into skip_ignore_source_setting

92bdc92

Revert most of test changes

066f001

Simpler approach

ccb03c5

martijnvg approved these changes Sep 17, 2024

View reviewed changes

style

d08ad42

lkts merged commit 36b3549 into elastic:main Sep 17, 2024
14 checks passed

lkts deleted the skip_ignore_source_setting branch September 17, 2024 20:47

lkts mentioned this pull request Sep 17, 2024

[8.x] Add option to skip using _ignored_source field for synthetic source (#112963) #113057

Merged

lkts added a commit to lkts/elasticsearch that referenced this pull request Sep 17, 2024

Add option to skip using _ignored_source field for synthetic source (e…

aa9afce

…lastic#112963)

elasticsearchmachine added the backport pending label Sep 17, 2024

lkts mentioned this pull request Sep 17, 2024

[8.15] Add option to skip using _ignored_source field for synthetic source (#112963) #113065

Merged

kkrik-es reviewed Sep 18, 2024

View reviewed changes

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

javanna pushed a commit to javanna/elasticsearch that referenced this pull request Sep 18, 2024

Add option to skip using _ignored_source field for synthetic source (e…

d462f41

…lastic#112963)

elasticsearchmachine pushed a commit that referenced this pull request Sep 18, 2024

Add option to skip using _ignored_source field for synthetic source (#…

88a4fb2

…112963) (#113057) Co-authored-by: Elastic Machine <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to skip using _ignored_source field for synthetic source #112963

Add option to skip using _ignored_source field for synthetic source #112963

lkts commented Sep 16, 2024

elasticsearchmachine commented Sep 16, 2024

lkts commented Sep 17, 2024

kkrik-es Sep 17, 2024 •

edited

Loading

kkrik-es Sep 17, 2024

kkrik-es Sep 17, 2024

martijnvg Sep 17, 2024

martijnvg Sep 17, 2024

lkts Sep 17, 2024

martijnvg Sep 17, 2024

kkrik-es Sep 17, 2024

martijnvg Sep 17, 2024

lkts commented Sep 17, 2024

martijnvg left a comment

martijnvg Sep 17, 2024

elasticsearchmachine commented Sep 17, 2024

lkts commented Sep 17, 2024


		static final NodeFeature TRACK_IGNORED_SOURCE = new NodeFeature("mapper.track_ignored_source");

		public static final Setting<Boolean> SKIP_IGNORED_SOURCE_WRITE_SETTING = Setting.boolSetting(

Add option to skip using _ignored_source field for synthetic source #112963

Add option to skip using _ignored_source field for synthetic source #112963

Conversation

lkts commented Sep 16, 2024

elasticsearchmachine commented Sep 16, 2024

lkts commented Sep 17, 2024

kkrik-es Sep 17, 2024 • edited Loading

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

lkts commented Sep 17, 2024

martijnvg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Sep 17, 2024

💔 Backport failed

lkts commented Sep 17, 2024

💚 All backports created successfully

Questions ?

kkrik-es Sep 17, 2024 •

edited

Loading