Allow single-valued updates for SortedNumericDocValues

[HoustonPutman]

Description

As mentioned in #16580, there are good reasons to want to use SortedNumericDocValues for singleValued fields. This PR adds support for issuing single-value updates for SortedNumericDocValues. Originally this was only to support single-valued SortedNumericDocValues, but really it's not hard to support multi-valued SortedNumericDocValues. Supporting multi-value updates is harder, so that will be left for a future enhancement.

[AdityaTeltia]

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.

[jimczi]

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.

[HoustonPutman]

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.

MergedDocValues also has both of the same issue, but I will just update the code to use MergedDocValues instead.

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.

Yeah, more tests were certainly needed. Added more.

1. Interaction with Stop buffering doc-values updates in heap during merges #16570 (merge carry-over): sorted-numeric updates that resolve during a merge are dropped or mis-typed.

I've merged and now it should support sorted-numeric updates. Also changed IndexWriter.addDiskDiffToPacket() to be per-dv-type.

[jimczi]

Thanks Houston, this all looks good now. The per-type split in addDiskDiffToPacket is clean and the sorted-numeric carry-over reads right: a still-multi-valued doc can't have been updated, so a single or absent value diffed against the base is exactly the set of updates that flushed during the merge.

Two small things.

The carry-over fix itself doesn't have a test yet. The new multi-valued tests cover the fold and dense rewrite, but nothing resolves an update while a merge is running, so addSortedNumericDiskDiffToPacket never actually runs. I wrote one on top of TestMergeCarryOverFromDisk (from #16570, it already has the paused-merge machinery) and it passes 30 iters. Feel free to grab it, just add import org.apache.lucene.document.SortedNumericDocValuesField;:

  /**
   * A single-valued sorted-numeric update that resolves onto a segment while it is being merged must
   * be carried over, and an untouched multi-valued doc must keep its whole value set. Exercises the
   * sorted-numeric branch of the disk carry-over (addSortedNumericDiskDiffToPacket).
   */
  public void testSortedNumericUpdateResolvedDuringMergeIsCarriedOver() throws Exception {
    MergePausingDirectory dir = new MergePausingDirectory(newDirectory());
    IndexWriterConfig conf =
        newIndexWriterConfig(new MockAnalyzer(random()))
            .setMergeScheduler(new ConcurrentMergeScheduler());
    IndexWriter writer = new IndexWriter(dir, conf);

    // First segment: doc 0 single-valued, doc 1 genuinely multi-valued (never updated).
    Document d0 = new Document();
    d0.add(new StringField("id", "0", StringField.Store.NO));
    d0.add(new SortedNumericDocValuesField("snv", 10));
    writer.addDocument(d0);
    Document d1 = new Document();
    d1.add(new StringField("id", "1", StringField.Store.NO));
    d1.add(new SortedNumericDocValuesField("snv", 20));
    d1.add(new SortedNumericDocValuesField("snv", 21));
    writer.addDocument(d1);
    writer.commit();
    // Second segment so the merge has two sources.
    for (int i = 2; i < 6; i++) {
      Document d = new Document();
      d.add(new StringField("id", Integer.toString(i), StringField.Store.NO));
      d.add(new SortedNumericDocValuesField("snv", i * 10L));
      writer.addDocument(d);
    }
    writer.commit();

    Thread merger =
        new Thread(
            () -> {
              try {
                writer.forceMerge(1);
              } catch (Throwable t) {
                dir.failure.compareAndSet(null, t);
              }
            },
            "forceMerge");
    merger.start();
    dir.mergeStarted.await();

    // Update a single-valued doc in each merging segment, then resolve them to disk.
    writer.updateSortedNumericDocValue(new Term("id", "0"), "snv", 100);
    writer.updateSortedNumericDocValue(new Term("id", "4"), "snv", 104);
    try (DirectoryReader r = DirectoryReader.open(writer)) {
      assertNotNull(r);
    }

    dir.resumeMerge.countDown();
    merger.join();
    assertNull("forceMerge failed: " + dir.failure.get(), dir.failure.get());

    try (DirectoryReader reader = DirectoryReader.open(writer)) {
      assertEquals(1, reader.leaves().size()); // single merged segment
      LeafReader leaf = reader.leaves().get(0).reader();
      SortedNumericDocValues snv = leaf.getSortedNumericDocValues("snv");
      Terms idTerms = leaf.terms("id");
      TermsEnum te = idTerms.iterator();
      java.util.Map<String, long[]> byId = new java.util.HashMap<>();
      BytesRef t;
      while ((t = te.next()) != null) {
        PostingsEnum pe = te.postings(null, PostingsEnum.NONE);
        int docId = pe.nextDoc();
        assertTrue(snv.advanceExact(docId));
        long[] vals = new long[snv.docValueCount()];
        for (int i = 0; i < vals.length; i++) {
          vals[i] = snv.nextValue();
        }
        byId.put(t.utf8ToString(), vals);
      }
      // The two updates resolved during the merge were carried over.
      assertArrayEquals("carried update for doc 0", new long[] {100}, byId.get("0"));
      assertArrayEquals("carried update for doc 4", new long[] {104}, byId.get("4"));
      // The untouched multi-valued doc kept its whole value set through the merge.
      assertArrayEquals("untouched multi-valued doc preserved", new long[] {20, 21}, byId.get("1"));
      // An untouched single-valued doc is unchanged.
      assertArrayEquals(new long[] {30}, byId.get("3"));
    }

    writer.close();
    dir.close();
  }

Also the comments in testDenseRewriteOverMultiValuedBase and testFoldToDenseOverMultiValuedBase still point at ReadersAndUpdates.MergedSortedNumericDocValues, which is gone now.

[HoustonPutman]

Fair enough @jimczi . Instead, I just added SortedNumericDocValues (both singleton and multi-valued) to the existing 3 tests in TestMergeCarryOverFromDisk. I also added some tests around singletons in TestSortedNumericDocValuesUpdates.

Also good call on the comments. Updated them throughout the file.

[jimczi]

LGTM, thanks for the thorough follow-ups. The extended TestMergeCarryOverFromDisk covers the sorted-numeric merge carry-over well now, single and multi-valued base across the plain, fold and soft-delete paths, and it is green here over iterations along with the full index package.

One note for @AdityaTeltia: the multi-valued rejection your comment pointed at is gone, the PR now supports a multi-valued base rather than throwing inside handleDVUpdates, so the stuck-packet path no longer exists. I think that thread can be resolved.

[AdityaTeltia]

Yes, we can mark it resolved now. Apparently resolving a review thread requires either write access to the repo or being the PR author. Thus, I am unable to see "Resolve Conversation/Comment" option.

[github-actions]

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!