VijayanB opened a new issue, #16427:
URL: https://github.com/apache/lucene/issues/16427
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
