john-mlika opened a new pull request, #16481:
URL: https://github.com/apache/lucene/pull/16481
<!-- TITLE: Backport #14977 to branch_10x: invoke
KnnVectorsReader#finishMerge() from SegmentMerger#cleanupMerge() -->
<!-- TARGET: apache/lucene branch_10x (PR) -->
<!-- LOCAL BRANCH: 10x-backport-14977-finishmerge @
john-ubuntu:/home/john/lucene-10x -->
<!-- COMMIT: 13a20e21836d4151b9495be13b199aabc281b76f -->
Closes #16419
### Problem
On `branch_10x`, `KnnVectorsReader#finishMerge()` is dead code — a sweep of
the branch finds only
its definitions (`KnnVectorsReader`, `Lucene99FlatVectorsReader`,
`Lucene99HnswVectorsReader`,
`PerFieldKnnVectorsFormat$FieldsReader`) and the `AssertingKnnVectorsFormat`
wrapper. There is no
call site.
The flip side of the machinery is fully wired. `MergeState` starts the
cascade
(`knnVectorsReaders[i] = knnVectorsReaders[i].getMergeInstance()`), and
`Lucene99FlatVectorsReader#getMergeInstance` implements it by switching the
**shared,
search-visible** raw `.vec` `IndexInput` to `DataAccessHint.SEQUENTIAL` — it
mutates and returns
`this`. Nothing ever switches it back. The advice sticks until the segment
reader is closed, so
pooled readers that concurrent searches share keep SEQUENTIAL advice long
after the merge is over,
and segments that survive as sources of an aborted merge keep it until close.
How the branch got here:
- #14977 fixed exactly this on `main` (`085a0ae054b`, its own rationale:
*"ReadAdvice not being
reset properly"*) by moving the cascade initiation into
`SegmentMerger#cleanupMerge()`, invoked
from both `IndexWriter` call sites inside `finally` blocks. It was never
backported here.
- `d1c4db43036` ("Prevent writing vectors twice during merging HNSW graphs",
the `branch_10x`
backport of #15732) **deleted the previous caller** — the private
`finishMerge(MergeState)`
helper and its `reader.finishMerge()` loop in `KnnVectorsWriter#merge()` —
without the #14977
replacement existing on the branch.
- `d1c4db43036` is an ancestor of `releases/lucene/10.5.0`, so 10.5.0
shipped with it.
This only bites Directory implementations that honour read advice (mmap); it
is a no-op elsewhere.
### Change
Backport of `085a0ae054b`, kept as close to the main commit as the branch
allows.
- `SegmentMerger#cleanupMerge()` restored (`SegmentMerger.java:331`) —
byte-identical to main's.
- Invoked from both `IndexWriter` call sites inside `finally` blocks: the
`addIndexes(CodecReader…)`
path (`IndexWriter.java:3587`) and the merge path
(`IndexWriter.java:5408`). Both `try`/`finally`
restructurings are line-for-line identical to the main commit's hunks.
- `SegmentMerger#mergeFieldInfos()` narrowed `public` → `private`, as in the
main commit. Cosmetic:
`SegmentMerger` is a package-private final class and the only caller is
internal (`:119`).
- `TestDoc` and `TestSegmentMerger` each gain the one
`merger.cleanupMerge()` line the main commit
added; both drive `SegmentMerger` directly rather than through
`IndexWriter`.
Two deviations from `085a0ae054b`, both forced by the branch:
1. **The `KnnVectorsWriter.java` hunk is omitted because it is already
applied.** On main, #14977
deleted the private `finishMerge(MergeState)` helper and its call from
`KnnVectorsWriter#merge()`
(−9 lines). On `branch_10x` `d1c4db43036` already deleted exactly that
code — that deletion is
what caused this bug. The file is therefore not touched, and the resulting
`KnnVectorsWriter#merge()` matches main's post-#14977 state.
2. **Diff context differs at both `IndexWriter` call sites**: the
`branch_10x` `SegmentMerger`
constructor takes an extra trailing `MergePolicy.OneMerge merge` argument
(from the #16368 /
#16391 backport) that main does not have. Context only — no change to the
backported lines.
Nothing else diverges. The post-state of `cleanupMerge()` and both `finally`
blocks is textually
identical to main.
### Test
New `TestMergeReadAdviceRevert`
(`lucene/core/src/test/org/apache/lucene/codecs/lucene99/`). It
indexes two segments, holds an NRT reader open across `forceMerge(1)`, and
records every
`updateIOContext` per file through a recording `FilterDirectory`, asserting
that each `.vec`
switched to SEQUENTIAL for the merge is switched back before its input
closes.
Three constraints, or it silently false-passes:
- the field must be a **plain float HNSW** field — on `branch_10x` the
quantized readers don't
override `getMergeInstance` at all, so a quantized detector would fail for
the wrong reason;
- an **NRT reader must be held open across the merge** — a merge-created
pooled reader is opened
with `IOContext.merge(...)`, whose `withHints` is a documented no-op, so
the flip never becomes
observable;
- the recording `IndexInput` must explicitly delegate `updateIOContext`,
`clone()` and **both**
`slice` overloads — `FilterIndexInput` forwards none of them.
It also carries a precondition assertion that fails loudly if no `.vec` ever
received a SEQUENTIAL
`updateIOContext`, so a future change that stops flipping advice cannot turn
this into a vacuous
pass.
**Before/after, verified in separate patched and stock worktrees** with the
exact same detector and
seed `648B326F8F9AF8DD`:
- Stock production code — **FAILS**:
```
java.lang.AssertionError: .vec inputs still using SEQUENTIAL advice:
_0_Lucene99HnswVectorsFormat_0.vec [ADVICE:SEQUENTIAL]
_1_Lucene99HnswVectorsFormat_0.vec [ADVICE:SEQUENTIAL]
```
One flip per source segment, no revert, and both inputs still open (pinned
by the NRT reader)
when the assertion runs.
- Fix restored — **PASSES**, same seed. The temporary stock worktree was
removed after its failure
log was preserved.
### AssertingKnnVectorsFormat
`AssertingKnnVectorsFormat`'s close assertion is tightened to what #14977
made it on main
(`AssertingKnnVectorsFormat.java:237`):
```java
- assert finishMergeCount.get() <= 0 || mergeInstanceCount.get() ==
finishMergeCount.get();
+ assert mergeInstanceCount.get() == finishMergeCount.get();
```
This is part of why the regression survived the backport: with the weaker
form, `AssertingCodec`
cannot catch a recurrence, and it did not catch this one.
It is not cosmetic. With `cleanupMerge()`'s body disabled and the strict
assertion in place, a full
`:lucene:core:test` run fails **27 tests across 9 suites** — 26 of them
tracing to
`AssertingKnnVectorsFormat.java:237` via
`AssertingKnnVectorsReader.close()`, plus the new detector:
```
TestPerFieldKnnVectorsFormat, TestColumnBatchVectorColumn, TestCheckIndex,
TestConcurrentMergeScheduler, TestKnnByteVectorQueryMMap,
TestKnnFloatVectorQuery,
TestPatienceByteVectorQuery, TestPatienceFloatVectorQuery,
TestSeededKnnByteVectorQuery
```
With the fix in place all of them pass. So the tightening catches the
genuine gap this PR closes,
not unrelated pre-existing breakage — it does not need to be split out.
### CHANGES.txt
Entry added under 10.6.0 → Bug Fixes, citing GITHUB#16419 and crediting
Simon Cooper (the original
#14977 author) alongside the backport.
### Validation
Rebased onto `branch_10x` at `2ab41d8d0f4` on 2026-08-03. A subsequent
comment-style pass amended
the commit to `58948f7aeac`; its range-diff against the pre-audit commit
changes only this test's
comments/assertion text and the `CHANGES.txt` wording. A final message-only
amendment removed the
commit body, producing `13a20e21836` with the same tree. All production
paths are byte-identical.
JDK 21 (Temurin 21.0.7), `./gradlew check` across every module — **37
modules, 19,777 tests,
1,461 skipped, 0 failures** (seed `F25239DD5384A691`):
| Module | Tests | Result |
| --- | --- | --- |
| `:lucene:core` | 8559 (265 skipped) | SUCCESS |
| `:lucene:backward-codecs` | 2939 (1015 skipped) | SUCCESS |
| `:lucene:analysis:common` | 1855 (19 skipped) | SUCCESS |
| `:lucene:spatial3d` | 712 (3 skipped) | SUCCESS |
| `:lucene:codecs` | 684 (65 skipped) | SUCCESS |
| `:lucene:test-framework` | 590 (30 skipped) | SUCCESS |
| … 31 further modules | | all SUCCESS |
Targeted vector-codec sweep (`codecs.lucene99.*`, `lucene95.*`,
`lucene94.*`, `lucene104.*`,
`lucene103.*`, `codecs.hnsw.*`, `codecs.perfield.*`,
`TestMergedVectorValues`): **687 tests,
23 skipped, 0 failures**.
The two direct `SegmentMerger` lifecycle suites (`TestDoc`,
`TestSegmentMerger`) add **4 tests,
0 failures**. `./gradlew tidy` produces no changes; the full `check`
completed 900 tasks successfully.
### Notes for review
- The advice-revert cascade is only reachable for readers that propagate
`getMergeInstance()`. On
this branch that is plain float/HNSW via
`Lucene99FlatVectorsReader`/`Lucene99HnswVectorsReader`;
the quantized readers don't override it at all, so they are shielded from
the bug by a second gap.
If that gap is closed on `branch_10x` (a separate change propagating
`getMergeInstance` through
the quantized readers), quantized fields would join this regression —
which is why that backport
should land after this one, not before.
- `cleanupMerge()` is invoked from a `finally` that begins *after* `new
SegmentMerger(...)` returns.
If the constructor itself throws partway through `MergeState`'s cascade,
merge instances already
obtained are not finished. That is the same on main; this backport does
not change it, and it
seems out of scope here, but a reviewer may want to note it.
--
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]
