Separate sequential-read from merge in StoredFieldsReader (allow different instances for read vs merge )
- Dominant language
- Java
- Stars
- 3.6k
- Forks
- 1.4k
- Avg merge
- 2d 11h
- Merged PRs (30d)
- 88
Description
### Description
`StoredFieldsReader.getMergeInstance()` is currently used for two conceptually different purposes, and there is no way for an implementation to tell them apart. I'd like to propose a distinct method (e.g. `getSequentialInstance()`) for the "read a contiguous run of documents efficiently" use case, separate from the existing "I am merging segments" use case, so that a `StoredFieldsReader` can return a different instance for read vs. merge.
### Background: one method, two meanings
`getMergeInstance()` is documented as *"an instance optimized for merging … may not be cloned."* Its performance value (in `Lucene90CompressingStoredFieldsReader`) is that it decompresses each block once into a reused buffer and serves all docs in that block from it — ideal for reading adjacent doc ids.
That property is useful well beyond merging, which is why it is also used at **read time**.
### How this is used in OpenSearch
OpenSearch fetches stored fields for a contiguous run of doc ids in several common paths — deep pagination, scroll, `match_all` with a large `size`, reindex, and source-heavy aggregations. When the docs to fetch are adjacent, OpenSearch calls `getMergeInstance()` to obtain the block-at-a-time reader and gets a significant speedup over the random-access path.
OpenSearch also wraps `StoredFieldsReader` to transform stored fields at read time. A concrete case is derived source, where large fields (e.g. vectors) are stripped from `_source` on write to save space and re-injected when `_source` is read back. In that model the desired behavior differs by call site:
- **Read** — the wrapper must **inject/transform** so the user gets complete `_source`.
- **Merge** — the wrapper must **not** inject/transform, so the stripped representation is copied through untouched and not re-materialized on disk.
The natural way to express this is:
- `getMergeInstance()` → transformation **off** (merge path)
- normal `document()` → transformation **on** (read path)
### The problem: read and merge are indistinguishable
Because the read-time sequential path *also* goes through `getMergeInstance()`, a `StoredFieldsReader` implementation cannot use `getMergeInstance()` to mean "I am merging." Overriding it to disable transformation would also disable transformation on ordinary read-time sequential fetches, which need it. The two intents collapse onto the same method, so a wrapper that legitimately wants merge ≠ read behavior has no way to express it.
### Why a separate method is justified (rather than reusing `getMergeInstance()`)
1. **They are different contracts.** "Optimize for merging" carries merge-specific semantics (may not be cloned, merge lifecycle, and — for wrapping readers — merge-copy behavior). "Optimize for sequential reads" is a read-time, user-facing path with different lifecycle and transformation expectations. A single method cannot honor both.
2. **It lets implementations decouple the two instances.** With a distinct entry point, a `StoredFieldsReader` can return one instance tuned for sequential reads (transformation on) and another for merging (transformation off), overriding each independently.
3. **The optimization is worth keeping — so "just stop using it for reads" isn't free.** Falling back to the random-access reader for sequential fetches is a real performance regression. The sequential-read use case needs its own sanctioned entry point, not removal.
### Proposal
Add a distinct entry point on `StoredFieldsReader` for sequential/bulk read access, separate from `getMergeInstance()`, with its own documented threading/cloning/lifecycle contract. Implementations that don't care can default it to the current behavior; implementations that need to distinguish read from merge can override the two independently.
```java
/**
* Returns an instance optimized for sequential access of adjacent doc ids at
* read time. Distinct from {@link #getMergeInstance()}, which signals merging,
* so implementations can return a different instance for reads vs. merges.
* The default implementation returns {@code this}.
*/
public StoredFieldsReader getSequentialInstance() {
return this;
}
```
### Prior art / related
- #10780 (LUCENE-9741) — earlier attempt to optimize sequential stored-field access; closed as the proposed API was considered too complex.
This proposal is narrower: rather than a prefetch/range API, it only asks to *disambiguate* the existing capability so reads and merges can use different instances.
Contributor guide
Research direction
Start by reading StoredFieldsReader and its getMergeInstance() contract, then inspect how Lucene90CompressingStoredFieldsReader uses that instance for sequential reads and merges. Trace the relevant read and merge call paths to determine the lifecycle, cloning, and threading expectations. Done means sequential reads and merges have distinct documented entry points while implementations that do not distinguish them retain the current behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend, search
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100