Address mapping and compute engine runtime field issues by martijnvg · Pull Request #117792 · elastic/elasticsearch

martijnvg · 2024-11-30T13:11:08Z

This PR addresses the following issues:

Fields mapped as runtime fields not getting stored if source mode is synthetic.
Address java.io.EOFException when an es|ql query uses multiple runtime fields that fallback to source when source mode is synthetic. (1)
Address concurrency issue when runtime fields get pushed down to Lucene. (2)

1: ValueSourceOperator can read values in row striding or columnar fashion. When values are read in columnar fashion and multiple runtime fields synthetize source then this can cause the same SourceProvider evaluation the same range of docs ids multiple times. This can then result in unexpected io errors at the codec level. This is because the same doc value instances are used by SourceProvider. Re-evaluating the same docids is in violation of the contract of the DocIdSetIterator#advance(...) / DocIdSetIterator#advanceExact(...) methods, which documents that unexpected behaviour can occur if target docid is lower than current docid position.

Note that this is only an issue for synthetic source loader and not for stored source loader. And not when executing in row stride fashion which sometimes happen in compute engine and always happen in _search api.

2: The concurrency issue that arrises with source provider if source operator executes in parallel with data portioning set to DOC. The same SourceProvider instance then gets access by multiple threads concurrently. SourceProviders implementations are not designed to handle concurrent access.

Closes #117644

This change attempts to solve the following issue: ValueSourceOperator can read values in row striding or columnar fashion. When values are read in columnar fashion and multiple runtime fields synthetize source then this can cause the same `SourceProvider` evaluation the same range of docs ids multiple times. This can then result in unexpected io errors at the codec level. This is because the same doc value instances are used by `SourceProvider`. Re-evaluating the same docids is in violation of the contract of the DocIdSetIterator#advance(...) / DocIdSetIterator#advanceExact(...) methods, which documents that unexpected behaviour can occur if target docid is lower than current docid position. Note that this is only an issue for synthetic source loader and not for stored source loader. And not when executing in row stride fashion which sometimes happen in compute engine and always happen in _search api. Closes elastic#117644

…ored_source!!!

…level.

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

martijnvg · 2024-12-02T15:46:40Z

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

    @Override
    public BlockLoader blockLoader(BlockLoaderContext blContext) {
-        return new LongScriptBlockDocValuesReader.LongScriptBlockLoader(leafFactory(blContext.lookup()));
+        boolean isSyntheticSource = SourceFieldMapper.isSynthetic(blContext.indexSettings());


Should we have two different implementations based on source mode?

martijnvg · 2024-12-02T15:54:10Z

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

+
+        @Override
+        public final StoredFieldsSpec rowStrideStoredFieldSpec() {
+            return StoredFieldsSpec.NEEDS_SOURCE;


This is needed because otherwise the follow error occurs:

[2024-12-03T04:52:11,002][WARN ][o.e.x.e.a.EsqlResponseListener] [test-cluster-0] Request failed with status [INTERNAL_SERVER_ERROR]: java.lang.IllegalStateException: found row stride readers [[RowStrideReaderWork[reader=ScriptLongs, builder=org.elasticsearch.compute.data.LongBlockBuilder@19f6840e, loader=org.elasticsearch.index.mapper.LongScriptBlockDocValuesReader$RowStrideLongScriptBlockLoader@6f332c9, offset=1], RowStrideReaderWork[reader=ScriptLongs, builder=org.elasticsearch.compute.data.LongBlockBuilder@12a1d61f, loader=org.elasticsearch.index.mapper.LongScriptBlockDocValuesReader$RowStrideLongScriptBlockLoader@7b7a5f6c, offset=2]]] without stored fields [StoredFieldsSpec[requiresSource=false, requiresMetadata=false, requiredStoredFields=[]]] at org.elasticsearch.compute.lucene.ValuesSourceReaderOperator.loadFromSingleLeaf(ValuesSourceReaderOperator.java:254) at org.elasticsearch.compute.lucene.ValuesSourceReaderOperator.process(ValuesSourceReaderOperator.java:143) at org.elasticsearch.compute.operator.AbstractPageMappingOperator.getOutput(AbstractPageMappingOperator.java:76) at org.elasticsearch.compute.operator.Driver.runSingleLoopIteration(Driver.java:258) at org.elasticsearch.compute.operator.Driver.run(Driver.java:189) at org.elasticsearch.compute.operator.Driver$1.doRun(Driver.java:378) at org.elasticsearch.server@9.0.0-SNAPSHOT/org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:27) at org.elasticsearch.server@9.0.0-SNAPSHOT/org.elasticsearch.common.util.concurrent.TimedRunnable.doRun(TimedRunnable.java:34) at org.elasticsearch.server@9.0.0-SNAPSHOT/org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:1023) at org.elasticsearch.server@9.0.0-SNAPSHOT/org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:27) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) at java.base/java.lang.Thread.run(Thread.java:1575)

However it isn't really required. Since the SearchLook / SourceLoader will load stored fields and not compute engine.

…te engine and scripting infrastructure to load _source

…base

dnhatn · 2024-12-04T06:56:50Z

Another issue is the thread switching, as the SyntheticSourceProvider is designed to work with only one thread. I think we need to address this before merging the change. It appears we are loading the source once for each script field, but I think it's okay to address the performance issue later.

сне 04, 2024 1:48:31 AM com.carrotsearch.randomizedtesting.RandomizedRunner$QueueUncaughtExceptionsHandler uncaughtException
WARNING: Uncaught exception in thread: Thread[#143,elasticsearch[node_s1][esql_worker][T#2],5,TGRP-SyntheticSourceIT]
java.lang.AssertionError: StoredFieldsReader are only supposed to be consumed in the thread in which they have been acquired. But was acquired in Thread[#145,elasticsearch[node_s1][esql_worker][T#4],5,TGRP-SyntheticSourceIT] and consumed in Thread[#143,elasticsearch[node_s1][esql_worker][T#2],5,TGRP-SyntheticSourceIT].
	at __randomizedtesting.SeedInfo.seed([273E3739EEB5DBA8]:0)
	at org.apache.lucene.tests.codecs.asserting.AssertingCodec.assertThread(AssertingCodec.java:44)
	at org.apache.lucene.tests.codecs.asserting.AssertingStoredFieldsFormat$AssertingStoredFieldsReader.document(AssertingStoredFieldsFormat.java:75)
	at org.apache.lucene.index.CodecReader$1.document(CodecReader.java:101)
	at org.elasticsearch.search.internal.FieldUsageTrackingDirectoryReader$FieldUsageTrackingLeafReader$2.document(FieldUsageTrackingDirectoryReader.java:137)
	at org.elasticsearch.index.fieldvisitor.StoredFieldLoader$ReaderStoredFieldLoader.advanceTo(StoredFieldLoader.java:205)
	at org.elasticsearch.search.lookup.SyntheticSourceProvider$SyntheticSourceLeafLoader.getSource(SyntheticSourceProvider.java:57)
	at org.elasticsearch.search.lookup.SyntheticSourceProvider.getSource(SyntheticSourceProvider.java:42)
	at org.elasticsearch.search.lookup.LeafSearchLookup.lambda$new$0(LeafSearchLookup.java:40)
	at org.elasticsearch.script.AbstractFieldScript.extractFromSource(AbstractFieldScript.java:107)
	at org.elasticsearch.script.AbstractFieldScript.emitFromSource(AbstractFieldScript.java:127)
	at org.elasticsearch.script.LongFieldScript$1$1.execute(LongFieldScript.java:29)
	at org.elasticsearch.script.AbstractFieldScript.runForDoc(AbstractFieldScript.java:159)
	at org.elasticsearch.index.mapper.LongScriptBlockDocValuesReader.read(LongScriptBlockDocValuesReader.java:69)
	at org.elasticsearch.index.mapper.LongScriptBlockDocValuesReader.read(LongScriptBlockDocValuesReader.java:65)
	at org.elasticsearch.compute.lucene.ValuesSourceReaderOperator$LoadFromMany.read(ValuesSourceReaderOperator.java:420)
	at org.elasticsearch.compute.lucene.ValuesSourceReaderOperator$LoadFromMany.run(ValuesSourceReaderOperator.java:359)
	at org.elasticsearch.compute.lucene.ValuesSourceReaderOperator.process(ValuesSourceReaderOperator.java:159)
	at org.elasticsearch.compute.operator.AbstractPageMappingOperator.getOutput(AbstractPageMappingOperator.java:76)
	at org.elasticsearch.compute.operator.Driver.runSingleLoopIteration(Driver.java:258)
	at org.elasticsearch.compute.operator.Driver.run(Driver.java:189)
	at org.elasticsearch.compute.operator.Driver$1.doRun(Driver.java:378)
	at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:27)
	at org.elasticsearch.common.util.concurrent.TimedRunnable.doRun(TimedRunnable.java:34)
	at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:1023)
	at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:27)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
	at java.base/java.lang.Thread.run(Thread.java:1575)

…w SourceLoader for each runtime field used a the query.

…ocid data partitioning and accesses _source (either synthetic or stored source)

elasticsearchmachine · 2024-12-05T06:29:22Z

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

server/src/main/java/org/elasticsearch/search/lookup/ReinitializingSourceProvider.java

dnhatn

These two workarounds are good! Feel free to merge the PR. Thanks, Martijn!

This change addresses the following issues: Fields mapped as runtime fields not getting stored if source mode is synthetic. Address java.io.EOFException when an es|ql query uses multiple runtime fields that fallback to source when source mode is synthetic. (1) Address concurrency issue when runtime fields get pushed down to Lucene. (2) 1: ValueSourceOperator can read values in row striding or columnar fashion. When values are read in columnar fashion and multiple runtime fields synthetize source then this can cause the same SourceProvider evaluation the same range of docs ids multiple times. This can then result in unexpected io errors at the codec level. This is because the same doc value instances are used by SourceProvider. Re-evaluating the same docids is in violation of the contract of the DocIdSetIterator#advance(...) / DocIdSetIterator#advanceExact(...) methods, which documents that unexpected behaviour can occur if target docid is lower than current docid position. Note that this is only an issue for synthetic source loader and not for stored source loader. And not when executing in row stride fashion which sometimes happen in compute engine and always happen in _search api. 2: The concurrency issue that arrises with source provider if source operator executes in parallel with data portioning set to DOC. The same SourceProvider instance then gets access by multiple threads concurrently. SourceProviders implementations are not designed to handle concurrent access. Closes elastic#117644

elasticsearchmachine · 2024-12-05T08:07:03Z

💚 Backport successful

Status	Branch	Result
✅	8.17
✅	8.x

This change addresses the following issues: Fields mapped as runtime fields not getting stored if source mode is synthetic. Address java.io.EOFException when an es|ql query uses multiple runtime fields that fallback to source when source mode is synthetic. (1) Address concurrency issue when runtime fields get pushed down to Lucene. (2) 1: ValueSourceOperator can read values in row striding or columnar fashion. When values are read in columnar fashion and multiple runtime fields synthetize source then this can cause the same SourceProvider evaluation the same range of docs ids multiple times. This can then result in unexpected io errors at the codec level. This is because the same doc value instances are used by SourceProvider. Re-evaluating the same docids is in violation of the contract of the DocIdSetIterator#advance(...) / DocIdSetIterator#advanceExact(...) methods, which documents that unexpected behaviour can occur if target docid is lower than current docid position. Note that this is only an issue for synthetic source loader and not for stored source loader. And not when executing in row stride fashion which sometimes happen in compute engine and always happen in _search api. 2: The concurrency issue that arrises with source provider if source operator executes in parallel with data portioning set to DOC. The same SourceProvider instance then gets access by multiple threads concurrently. SourceProviders implementations are not designed to handle concurrent access. Closes elastic#117644

…) (#118048) * Address mapping and compute engine runtime field issues (#117792) This change addresses the following issues: Fields mapped as runtime fields not getting stored if source mode is synthetic. Address java.io.EOFException when an es|ql query uses multiple runtime fields that fallback to source when source mode is synthetic. (1) Address concurrency issue when runtime fields get pushed down to Lucene. (2) 1: ValueSourceOperator can read values in row striding or columnar fashion. When values are read in columnar fashion and multiple runtime fields synthetize source then this can cause the same SourceProvider evaluation the same range of docs ids multiple times. This can then result in unexpected io errors at the codec level. This is because the same doc value instances are used by SourceProvider. Re-evaluating the same docids is in violation of the contract of the DocIdSetIterator#advance(...) / DocIdSetIterator#advanceExact(...) methods, which documents that unexpected behaviour can occur if target docid is lower than current docid position. Note that this is only an issue for synthetic source loader and not for stored source loader. And not when executing in row stride fashion which sometimes happen in compute engine and always happen in _search api. 2: The concurrency issue that arrises with source provider if source operator executes in parallel with data portioning set to DOC. The same SourceProvider instance then gets access by multiple threads concurrently. SourceProviders implementations are not designed to handle concurrent access. Closes #117644 * fixed compile error after backporting

… (#118049) * Address mapping and compute engine runtime field issues (#117792) This change addresses the following issues: Fields mapped as runtime fields not getting stored if source mode is synthetic. Address java.io.EOFException when an es|ql query uses multiple runtime fields that fallback to source when source mode is synthetic. (1) Address concurrency issue when runtime fields get pushed down to Lucene. (2) 1: ValueSourceOperator can read values in row striding or columnar fashion. When values are read in columnar fashion and multiple runtime fields synthetize source then this can cause the same SourceProvider evaluation the same range of docs ids multiple times. This can then result in unexpected io errors at the codec level. This is because the same doc value instances are used by SourceProvider. Re-evaluating the same docids is in violation of the contract of the DocIdSetIterator#advance(...) / DocIdSetIterator#advanceExact(...) methods, which documents that unexpected behaviour can occur if target docid is lower than current docid position. Note that this is only an issue for synthetic source loader and not for stored source loader. And not when executing in row stride fashion which sometimes happen in compute engine and always happen in _search api. 2: The concurrency issue that arrises with source provider if source operator executes in parallel with data portioning set to DOC. The same SourceProvider instance then gets access by multiple threads concurrently. SourceProviders implementations are not designed to handle concurrent access. Closes #117644

The previous fix to ensure that each thread uses its own SearchProvider wasn't good enough. When multiple threads access `ReinitializingSourceProvider` the simple thread accounting could still result in returned `SourceProvider` being used by multiple threads concurrently. The ReinitializingSourceProvider was introduced via elastic#117792 Closes elastic#118238

This reverts the workaround that was introduced in elastic#117792 to avoid EOF error when an es|ql query uses multiple runtime fields that fallback to source when source mode is synthetic. This is now covered by the `ReinitializingSourceProvider` workaround that covers that and the concurrency problem. With this change, the main code for the required workarounds are now in isolated in ReinitializingSourceProvider.

This reverts the workaround that was introduced in #117792 to avoid EOF error when an es|ql query uses multiple runtime fields that fallback to source when source mode is synthetic. This is now covered by the ReinitializingSourceProvider workaround that covers that and the concurrency problem. With this change, the main code for the required workarounds are now in isolated in ReinitializingSourceProvider. Additional another in `ReinitializingSourceProvider` was fixed, the issue was the lastSeenDoc field was reused overwritten by different threads, the latest commit moves the lastSeenDoc field to PerThreadSourceProvider so that each thread gets its own place to store the last seen docid.

This reverts the workaround that was introduced in elastic#117792 to avoid EOF error when an es|ql query uses multiple runtime fields that fallback to source when source mode is synthetic. This is now covered by the ReinitializingSourceProvider workaround that covers that and the concurrency problem. With this change, the main code for the required workarounds are now in isolated in ReinitializingSourceProvider. Additional another in `ReinitializingSourceProvider` was fixed, the issue was the lastSeenDoc field was reused overwritten by different threads, the latest commit moves the lastSeenDoc field to PerThreadSourceProvider so that each thread gets its own place to store the last seen docid.

Backporting #118480 to 8.x branch. This reverts the workaround that was introduced in #117792 to avoid EOF error when an es|ql query uses multiple runtime fields that fallback to source when source mode is synthetic. This is now covered by the ReinitializingSourceProvider workaround that covers that and the concurrency problem. With this change, the main code for the required workarounds are now in isolated in ReinitializingSourceProvider. Additional another in `ReinitializingSourceProvider` was fixed, the issue was the lastSeenDoc field was reused overwritten by different threads, the latest commit moves the lastSeenDoc field to PerThreadSourceProvider so that each thread gets its own place to store the last seen docid.

elasticsearchmachine added the v9.0.0 label Nov 30, 2024

martijnvg added 7 commits November 30, 2024 14:11

spotless

ea8d11c

reword

19fd9af

Merge remote-tracking branch 'es/main' into fix_117644

ad224b6

add integration test and ensure that runtime fields get added to _ign…

754c62c

…ored_source!!!

improve test so that it fails without the fix

2bee4d8

spotless

050d4b2

A more contained approach that resolve the issue at the block loader …

a47dfc6

…level.

martijnvg commented Dec 2, 2024

View reviewed changes

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

martijnvg commented Dec 2, 2024

View reviewed changes

martijnvg added 3 commits December 2, 2024 16:47

iter

ca0e1a5

iter2

e378530

need source

c09207b

martijnvg commented Dec 2, 2024

View reviewed changes

introduce StoredFieldsSpec.SCRIPT_NO_REQUIREMENTS to avoid both compu…

fdee715

…te engine and scripting infrastructure to load _source

martijnvg requested a review from dnhatn December 2, 2024 16:34

martijnvg added 6 commits December 2, 2024 20:56

keep one bock loader impl and fixed long script unit test

7be360c

Merge remote-tracking branch 'es/main' into fix_117644

848523b

spotless

7529bdf

adapt test

ec88323

add new constructor to reduce changes in unrelated areas of the code …

2aecb15

…base

Merge remote-tracking branch 'es/main' into fix_117644

4344fdb

martijnvg added 6 commits December 4, 2024 10:44

Revert block loader based solution for the solution that creates a ne…

f189cc1

…w SourceLoader for each runtime field used a the query.

Merge remote-tracking branch 'es/main' into fix_117644

130a96e

add potential fix for when compute engine executes in parallel with d…

611b50e

…ocid data partitioning and accesses _source (either synthetic or stored source)

spotless

21dc0a2

apply ReinitializingSourceProvider only for esql

0d515bd

spotless

dc398ae

dnhatn reviewed Dec 5, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/search/lookup/ReinitializingSourceProvider.java Outdated Show resolved Hide resolved

dnhatn approved these changes Dec 5, 2024

View reviewed changes

dnhatn mentioned this pull request Dec 5, 2024

Avoid pushing TopN of runtime Fields to Lucene #118043

Open

martijnvg added 2 commits December 5, 2024 07:49

remove duplicate copyright header

7202757

A less expensive approach to solve the concurrency issue

809a27b

martijnvg changed the title ~~Avoid reusing source providers for script based block loaders.~~ Dec 5, 2024

martijnvg changed the title ~~Address SourceProvider issues in compute engine.~~ Dec 5, 2024

martijnvg changed the title ~~Address runtime field issues~~ Dec 5, 2024

update changelog entry

bae95e4

martijnvg added the auto-backport Automatically create backport pull requests when merged label Dec 5, 2024

martijnvg merged commit 2051401 into elastic:main Dec 5, 2024

This was referenced Dec 5, 2024

[8.17] Address mapping and compute engine runtime field issues (#117792) #118048

Merged

[8.x] Address mapping and compute engine runtime field issues (#117792) #118049

Merged

dnhatn mentioned this pull request Dec 9, 2024

[CI] EsqlActionBreakerIT class failing #118238

Closed

martijnvg mentioned this pull request Dec 10, 2024

Fix issues with ReinitializingSourceProvider #118370

Merged

martijnvg mentioned this pull request Dec 11, 2024

Cleanup swapSourceProvider(...) workaround #118480

Merged

lkts mentioned this pull request Jan 6, 2025

Introduce FallbackSyntheticSourceBlockLoader and apply it to keyword fields #119546

Merged

dnhatn mentioned this pull request Jan 6, 2025

Introduce BlockLoaderTestCase #119415

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address mapping and compute engine runtime field issues#117792

Address mapping and compute engine runtime field issues#117792
martijnvg merged 30 commits intoelastic:mainfrom
martijnvg:fix_117644

martijnvg commented Nov 30, 2024 •

edited

Loading

Uh oh!

martijnvg Dec 2, 2024

martijnvg Dec 2, 2024

dnhatn commented Dec 4, 2024

elasticsearchmachine commented Dec 5, 2024

Uh oh!

dnhatn left a comment •

edited

Loading

elasticsearchmachine commented Dec 5, 2024

Labels

3 participants

Conversation

martijnvg commented Nov 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnvg Dec 2, 2024

Choose a reason for hiding this comment

martijnvg Dec 2, 2024

Choose a reason for hiding this comment

dnhatn commented Dec 4, 2024

elasticsearchmachine commented Dec 5, 2024

Uh oh!

dnhatn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

elasticsearchmachine commented Dec 5, 2024

💚 Backport successful

Labels

3 participants

martijnvg commented Nov 30, 2024 •

edited

Loading

dnhatn left a comment •

edited

Loading