AdityaTeltia commented on code in PR #16581:
URL: https://github.com/apache/lucene/pull/16581#discussion_r3888730066
##########
lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java:
##########
@@ -367,6 +369,24 @@ private synchronized void handleDVUpdates(
// back to the dense
// rewrite. (Skip-indexed fields can't reach here: IndexWriter rejects
doc-values updates on
// them.)
+ // A single-valued sorted-numeric field is stored as a numeric column
and reuses the numeric
+ // update path; a genuinely multi-valued column can't be updated in
place, so reject it here.
+ // This is the universal choke point for every update path (buffered
resolve, NRT
+ // tryUpdateDocValue and merge carry-over all funnel through here), and
the earliest place we
+ // hold a reader to know the column's cardinality. An active overlay
(existingOverlay != null)
+ // is single-valued by construction, so we only need to check the base
column.
+ if (type == DocValuesType.SORTED_NUMERIC && existingOverlay == null) {
+ SortedNumericDocValues baseDV =
reader.getSortedNumericDocValues(field);
+ if (baseDV != null && DocValues.unwrapSingleton(baseDV) == null) {
+ throw new IllegalArgumentException(
Review Comment:
This rejection happens at write time, but `writeFieldUpdates` only prunes
`pendingDVUpdates` after `handleDVUpdates` succeeds. The catch block rethrows
and the pruning loop below it never runs, so the offending
`DocValuesFieldUpdates` stays in `pendingDVUpdates` and nothing ever removes
it. Every subsequent commit, NRT reopen, merge, and RAM-triggered flush
re-enters this loop and throws again. The writer can't commit, refresh, or
`close()`, and the only recovery is `rollback()`, which discards everything
since the last commit. Since `handleDVUpdates` iterates all fields under one
`DocValuesConsumer`, unrelated valid NUMERIC/BINARY updates on the segment are
blocked permanently too.
`testUpdateOverMultiValuedBaseRejected` encodes this as expected behaviour,
but from the caller's side it's a data-loss trap: `updateSortedNumericDocValue`
returned a seqNo successfully, and the failure surfaces later in an unrelated
operation. In Solr/ES it's reachable from ordinary traffic, since one document
anywhere in the segment with two values wedges the writer. The check is also
per-segment, so the same update can be written to segment A and then throw on
segment B (partial application, no undo), and it can first fire during a
background merge via `commitMergedDeletesAndUpdates`.
I think validation needs to move to the
`IndexWriter.updateSortedNumericDocValue` / `updateDocValues` boundary so the
caller fails fast and nothing is ever buffered. If it has to stay here, the
minimum is dropping the offending entry from `pendingDVUpdates` before throwing
so the writer stays usable.
--
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]
