apache / apache/lucene

Separate sequential-read from merge in StoredFieldsReader (allow different instances for read vs merge )

Open
#16,427 0 comments 0 reactions 0 assignees View on GitHub
type:enhancement
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.