[I] Separate sequential-read from merge in StoredFieldsReader (allow different instances for read vs merge [lucene]

["VijayanB (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@lucene.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org