Is your feature request related to a problem? Please describe
SequentialStoredFieldsLeafReader.getSequentialStoredFieldsReader() obtains a sequential stored-fields reader by calling Lucene's StoredFieldsReader.getMergeInstance():
https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/common/lucene/index/SequentialStoredFieldsLeafReader.java#L74
return doGetSequentialStoredFieldsReader(reader.getFieldsReader().getMergeInstance());
This is a search/read-path optimization (used by FetchPhase for sequential fetches of adjacent docs), but it reuses getMergeInstance() - a method whose contract is "give me an instance for a merge." That overload means a codec's stored-fields reader cannot customize its behavior for merges without also changing what search fetch sees, because both callers arrive through the same method.
This has caused real correctness bugs for codecs that need different read-vs-merge behavior. The k-NN derived-source codec is one example: it must reconstruct vectors into _source on read but must not on merge (otherwise the reconstructed vector is written back to _source, defeating the feature). Because the read path borrows getMergeInstance(), k-NN cannot override getMergeInstance() to be non-injecting for merges - doing so returns the non-injecting reader to search too. This is why a prior attempt was reverted (opensearch-project/k-NN#2472), and it is the root cause of opensearch-project/k-NN#3316.
Describe the solution you'd like
Give the read path its own method so getMergeInstance() can mean only "merge" again.
- Add a small, OpenSearch-owned interface (e.g. in
org.opensearch.common.lucene.index):
public interface DelegatingStoredFieldsReader {
/** The wrapped delegate, for callers that need to see through decorators. */
StoredFieldsReader getDelegate();
/**
* Stored-fields instance for the sequential search/read path. Distinct in intent from
* Lucene's getMergeInstance(). Defaults to getMergeInstance() so behavior is unchanged unless
* an implementer overrides it.
*/
default StoredFieldsReader getSequentialInstance() {
StoredFieldsReader r = getDelegate();
return (r instanceof DelegatingStoredFieldsReader d) ? d.getSequentialInstance() : r.getMergeInstance();
}
}
- Have the sequential read path prefer
getSequentialInstance() when available, falling back to getMergeInstance() otherwise (behavior-preserving by default):
StoredFieldsReader sf = reader.getFieldsReader();
StoredFieldsReader sequentialInstance =
(sf instanceof DelegatingStoredFieldsReader d) ? d.getSequentialInstance() : sf.getMergeInstance();
return doGetSequentialStoredFieldsReader(sequentialInstance);
- Implement the interface on core's stored-fields decorators that wrap a delegate (e.g. the
RecoverySourcePruneMergePolicy stored-fields reader), returning the wrapped reader from getDelegate() and inheriting the default getSequentialInstance().
With this in place, a codec (e.g. k-NN) can override getMergeInstance() for merge-specific behavior and implement getSequentialInstance() for the read path - the two are no longer conflated. The change is behavior-preserving by default: any reader that does not implement the interface resolves getSequentialInstance() to getMergeInstance(), exactly as today.
The corresponding plugin-side change (k-NN overriding getMergeInstance() / implementing getSequentialInstance()) will be tracked in the k-NN repository and consumes this core change.
Related component
Other
Describe alternatives you've considered
Additional context
Is your feature request related to a problem? Please describe
SequentialStoredFieldsLeafReader.getSequentialStoredFieldsReader()obtains a sequential stored-fields reader by calling Lucene'sStoredFieldsReader.getMergeInstance():https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/common/lucene/index/SequentialStoredFieldsLeafReader.java#L74
This is a search/read-path optimization (used by
FetchPhasefor sequential fetches of adjacent docs), but it reusesgetMergeInstance()- a method whose contract is "give me an instance for a merge." That overload means a codec's stored-fields reader cannot customize its behavior for merges without also changing what search fetch sees, because both callers arrive through the same method.This has caused real correctness bugs for codecs that need different read-vs-merge behavior. The k-NN derived-source codec is one example: it must reconstruct vectors into
_sourceon read but must not on merge (otherwise the reconstructed vector is written back to_source, defeating the feature). Because the read path borrowsgetMergeInstance(), k-NN cannot overridegetMergeInstance()to be non-injecting for merges - doing so returns the non-injecting reader to search too. This is why a prior attempt was reverted (opensearch-project/k-NN#2472), and it is the root cause of opensearch-project/k-NN#3316.Describe the solution you'd like
Give the read path its own method so
getMergeInstance()can mean only "merge" again.org.opensearch.common.lucene.index):getSequentialInstance()when available, falling back togetMergeInstance()otherwise (behavior-preserving by default):RecoverySourcePruneMergePolicystored-fields reader), returning the wrapped reader fromgetDelegate()and inheriting the defaultgetSequentialInstance().With this in place, a codec (e.g. k-NN) can override
getMergeInstance()for merge-specific behavior and implementgetSequentialInstance()for the read path - the two are no longer conflated. The change is behavior-preserving by default: any reader that does not implement the interface resolvesgetSequentialInstance()togetMergeInstance(), exactly as today.The corresponding plugin-side change (k-NN overriding
getMergeInstance()/ implementinggetSequentialInstance()) will be tracked in the k-NN repository and consumes this core change.Related component
Other
Describe alternatives you've considered
getMergeInstance()entirely and useprefetch()for sequential access (Replace usage of SequentialStoredFieldsReader#getMergeInstance() in search path with prefetch #21583). This is the more complete fix - it makesgetMergeInstance()merge-only for all consumers - but it is a larger change across the search path. The interface above is a smaller, self-contained, behavior-preserving step that unblocks the k-NN bug now and is compatible with that future direction (once Replace usage of SequentialStoredFieldsReader#getMergeInstance() in search path with prefetch #21583 lands,getSequentialInstance()simply becomes unnecessary).Additional context
SequentialStoredFieldsLeafReader.java:74.FetchPhasefor sequential fetches of >= 10 adjacent docs._source.excludewith index mapping for derived source k-NN#3316getMergeInstance()directly is unsafe today): Switch away from getMergeInstance for derived source k-NN#2472