jimczi commented on PR #16581: URL: https://github.com/apache/lucene/pull/16581#issuecomment-5493544915
Nice change, and I think this is the right direction. Routing a single-valued sorted-numeric update through the existing numeric update object and buffer keeps it small, and the singleton unwrap in `SegmentDocValuesProducer.getSortedNumeric` / `OverlaySortedNumericDocValues.from` means single-valued columns pay nothing extra. A few things before merge; the first is a correctness issue that comes from a change that just landed on main. **1. Interaction with #16570 (merge carry-over): sorted-numeric updates that resolve during a merge are dropped or mis-typed.** #16570 removed the in-heap `mergingDVUpdates` buffer and now reconstructs the merge carry-over from disk in `buildMappedDVUpdatesFromDisk`. Two consequences after rebasing onto main: - The `case SORTED_NUMERIC` this PR adds to the switch in `commitMergedDeletesAndUpdates` no longer applies; that switch was replaced by the disk reconstruction. - `buildMappedDVUpdatesFromDisk` only understands NUMERIC and BINARY. The updated-field detection skips any field whose type is not NUMERIC or BINARY, so a SORTED_NUMERIC field is never detected and an update flushed mid-merge is dropped from the merged segment. `addDiskDiffToPacket` reads via `getNumericDocValues`, which is null for a sorted-numeric field. And the residual (resolved but not yet flushed) packets are typed as NUMERIC, so a sorted-numeric residual loses its type. Net effect: an `updateSortedNumericDocValue` that lands on a segment while that segment is being merged is silently lost or written with the wrong type. It only bites in the merge-carry-over window, so the current tests pass; it would fail under concurrent update + merge. To fix on rebase: add SORTED_NUMERIC to the updated-field type filter; in `addDiskDiffToPacket` read via `getSortedNumericDocValues` and unwrap the singleton (updatable implies single-valued, so `DocValues.unwrapSingleton` is non-null), building the packet with the new `NumericDocValuesFieldUpdates(delGen, field, SORTED_NUMERIC, maxDoc)` ctor; type the residual packet from `u.type`. A `TestMergeCarryOverFromDisk`-style test that resolves a sorted-numeric update onto a paused merge (single and multi-valued base) would lock it. Happy to point at the exact spots since I wrote #16570. **2. The multi-valued-base merge is the trickiest new code and is barely covered.** `testRandom` and both fold tests start docs single-valued (`doc(i, i)`), so they take the `singletonOnDisk != null` numeric branch and never reach `MergedSortedNumericDocValues`. That class is only exercised by two small deterministic cases with no fold and no stacking. Seeding some genuinely multi-valued, un-updated docs in `testRandom` and running with a low `maxDocValuesOverlays` would drive the folded rewrite over a multi-valued base and check it against the model. **3. The removal branch in `MergedSortedNumericDocValues` looks unreachable.** `updateSortedNumericDocValue` always sets a value and the `Field[]` path reads `f.numericValue()`, so nothing produces a sorted-numeric removal and `anyRemoval` is always false for this type. The `hasValue == false` skip loop is then dead. Either drop it (and assert `hasValue`), or keep it with a comment that it is currently unreachable, so it does not read as covered behavior. **4. `advance` / `advanceExact` throw on a `DocIdSetIterator`.** Safe today (the codec flush and the default `intoBitSet` only walk `nextDoc`), but it is a latent trap and diverges from the numeric sibling that supports both via `MergedDocValues`. Either implement `advance` by delegating, or note "flush-only, forward iteration" so the next caller is not surprised. Same thought on overriding `intoBitSet` for parity with the numeric path. **5. Nit:** `DocValues.isSingleton(SortedNumericDocValues)` is new public API but only used within the package; package-private avoids locking in surface area. -- 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]
