Skip to content

Commit 10100a9

Browse files
authored
[8.x] Cleanup swapSourceProvider(...) workaround (#120095)
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.
1 parent d918208 commit 10100a9

File tree

3 files changed

+14
-29
lines changed

3 files changed

+14
-29
lines changed

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

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -102,14 +102,6 @@ private SearchLookup(SearchLookup searchLookup, Set<String> fieldChain) {
102102
this.fieldLookupProvider = searchLookup.fieldLookupProvider;
103103
}
104104

105-
private SearchLookup(SearchLookup searchLookup, SourceProvider sourceProvider, Set<String> fieldChain) {
106-
this.fieldChain = Collections.unmodifiableSet(fieldChain);
107-
this.sourceProvider = sourceProvider;
108-
this.fieldTypeLookup = searchLookup.fieldTypeLookup;
109-
this.fieldDataLookup = searchLookup.fieldDataLookup;
110-
this.fieldLookupProvider = searchLookup.fieldLookupProvider;
111-
}
112-
113105
/**
114106
* Creates a copy of the current {@link SearchLookup} that looks fields up in the same way, but also tracks field references
115107
* in order to detect cycles and prevent resolving fields that depend on more than {@link #MAX_FIELD_CHAIN_DEPTH} other fields.
@@ -153,7 +145,4 @@ public Source getSource(LeafReaderContext ctx, int doc) throws IOException {
153145
return sourceProvider.getSource(ctx, doc);
154146
}
155147

156-
public SearchLookup swapSourceProvider(SourceProvider sourceProvider) {
157-
return new SearchLookup(this, sourceProvider, fieldChain);
158-
}
159148
}

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsPhysicalOperationProviders.java

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@
3333
import org.elasticsearch.index.mapper.FieldNamesFieldMapper;
3434
import org.elasticsearch.index.mapper.MappedFieldType;
3535
import org.elasticsearch.index.mapper.NestedLookup;
36-
import org.elasticsearch.index.mapper.SourceFieldMapper;
3736
import org.elasticsearch.index.mapper.SourceLoader;
3837
import org.elasticsearch.index.query.QueryBuilder;
3938
import org.elasticsearch.index.query.QueryBuilders;
@@ -342,16 +341,7 @@ public MappedFieldType.FieldExtractPreference fieldExtractPreference() {
342341

343342
@Override
344343
public SearchLookup lookup() {
345-
boolean syntheticSource = SourceFieldMapper.isSynthetic(indexSettings());
346-
var searchLookup = ctx.lookup();
347-
if (syntheticSource) {
348-
// in the context of scripts and when synthetic source is used the search lookup can't always be reused between
349-
// users of SearchLookup. This is only an issue when scripts fallback to _source, but since we can't always
350-
// accurately determine whether a script uses _source, we should do this for all script usages.
351-
// This lookup() method is only invoked for scripts / runtime fields, so it is ok to do here.
352-
searchLookup = searchLookup.swapSourceProvider(ctx.createSourceProvider());
353-
}
354-
return searchLookup;
344+
return ctx.lookup();
355345
}
356346

357347
@Override

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/ReinitializingSourceProvider.java

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,6 @@ final class ReinitializingSourceProvider implements SourceProvider {
2828
private PerThreadSourceProvider perThreadProvider;
2929
private final Supplier<SourceProvider> sourceProviderFactory;
3030

31-
// Keeping track of last seen doc and if current doc is before last seen doc then source provider is initialized:
32-
// (when source mode is synthetic then _source is read from doc values and doc values don't support going backwards)
33-
private int lastSeenDocId;
34-
3531
ReinitializingSourceProvider(Supplier<SourceProvider> sourceProviderFactory) {
3632
this.sourceProviderFactory = sourceProviderFactory;
3733
}
@@ -40,15 +36,25 @@ final class ReinitializingSourceProvider implements SourceProvider {
4036
public Source getSource(LeafReaderContext ctx, int doc) throws IOException {
4137
var currentThread = Thread.currentThread();
4238
PerThreadSourceProvider provider = perThreadProvider;
43-
if (provider == null || provider.creatingThread != currentThread || doc < lastSeenDocId) {
39+
if (provider == null || provider.creatingThread != currentThread || doc < provider.lastSeenDocId) {
4440
provider = new PerThreadSourceProvider(sourceProviderFactory.get(), currentThread);
4541
this.perThreadProvider = provider;
4642
}
47-
lastSeenDocId = doc;
43+
provider.lastSeenDocId = doc;
4844
return provider.source.getSource(ctx, doc);
4945
}
5046

51-
private record PerThreadSourceProvider(SourceProvider source, Thread creatingThread) {
47+
private static final class PerThreadSourceProvider {
48+
final SourceProvider source;
49+
final Thread creatingThread;
50+
// Keeping track of last seen doc and if current doc is before last seen doc then source provider is initialized:
51+
// (when source mode is synthetic then _source is read from doc values and doc values don't support going backwards)
52+
int lastSeenDocId;
53+
54+
private PerThreadSourceProvider(SourceProvider source, Thread creatingThread) {
55+
this.source = source;
56+
this.creatingThread = creatingThread;
57+
}
5258

5359
}
5460
}

0 commit comments

Comments
 (0)