fix: vector compaction lost commits, FLOAT signed zero delete, hash lookup at page end, Cypher mixed numeric index keys - #9132

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

 ___________________
< Look ma, no bugs! >
 -------------------
  \
   \   \
        \ /\
        ( )
      .( o ).

sequenceDiagram
  participant Writers
  participant MutableDataFile
  participant Compaction
  participant GraphBuild
  Compaction->>MutableDataFile: Acquire commit lock
  Compaction->>MutableDataFile: Read entries and rewrite vectors
  GraphBuild->>MutableDataFile: Read nextId after rewrite
  GraphBuild->>GraphBuild: Refresh delta snapshot
  Compaction->>MutableDataFile: Release commit lock
  Writers->>MutableDataFile: Commit writes

• Fix all pre-merge checks with AI

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

Not up to standards ⛔

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Review

I read the diff and the surrounding code. I did not build or run the tests. The fixes look well-targeted and each has a regression test. Findings are below, most important first.

Should address

1. Issue9071 test can silently stop compacting (Issue9071CompactionDuringConcurrentWritesTest, compaction thread). The try/catch sits outside the while (!stop.get()) loop. The first exception from COMPACT INDEX ends the thread with no trace, and the comment says "retried by the loop", which is not what happens. This PR also adds a new failure mode: lockDataFileForCompaction() throws TimeoutException when a writer holds the file lock for more than COMMIT_LOCK_TIMEOUT (5s by default, which is easy to hit on a 2-vCPU CI runner with 4 writers). One timeout early on leaves the test with zero compactions, and it passes anyway. Put the try/catch inside the loop. Also count successful compactions and assert the count is above zero, so the test cannot pass without exercising the fix.

2. The Cypher CREATE INDEX declares the property type from the first record: with integers and floats in the property, range queries drop rows once the index exists (x > 2.2 misses 2.5), and when the index build fails the declared type stays and coerces later writes (4.5 is stored as 4) #8999 rollback path has no test (failedIndexBuildLeavesNoDeclaredProperty). That test never makes the build fail: it creates the index successfully on mixed int/string data. The final assertion (declared == null || STRING) passes whether or not the new catch (RuntimeException) { dropProperty } in executeCreateIndex exists. Either force a build failure (for example a UNIQUE index over duplicate values) and assert the property is gone, or rename the test. It is also worth checking that dropping the property is right when the property was already declared by an earlier, concurrent statement. declaredHere only tracks this statement, so that case looks fine, but a test would pin it.

3. Order-dependent type inference for temporal values (inferPropertyTypeFromExistingData). A non-temporal value followed by a temporal one returns null ("a temporal value next to a value of another kind"). A temporal value followed by a non-temporal one is skipped with continue, so the property is declared DATETIME*. The same data set gives a different answer depending on record order. If the DATETIME_MICROS literal with a space separator and a fraction ('2024-02-29 13:45:10.123456', the psqlodbc bound-timestamp form) is silently stored as NULL #8090 reasoning only holds for the second case, consider making it symmetric, or at least add a test for both orders.

4. Existing STRING-keyed indexes change their key spelling (Type.convertIndexKeyOrNull, canonicalNumberKey). An index built before this PR over an undeclared numeric property holds keys like "3.0" or "1.0E10" on disk. After the PR, lookups use "3" and "10000000000", so those entries will not be found until a REBUILD INDEX. If that is acceptable, please say so in the PR description or release notes. BigDecimal.toPlainString() on an extreme-exponent value (1E+999999999) or a huge double such as 1e308 also allocates a very large string for a single key. Consider capping the scale or falling back to the plain toString() beyond some size.

Worth a look

5. Lock ordering in LSMVectorIndex. The compaction now takes graphBuildLock, then the data file commit lock (tryLockFile), then runs the whole page read and rewrite while holding the file lock. The reverse order, a committer holding the file lock and then waiting on graphBuildLock, would only show up as a timeout, since the lock is acquired with a timeout. The comment on REBUILD_SEMAPHORE and the tryLock() at ~L10095 suggest commits do not block on graphBuildLock, but please confirm that no commit-time path (for example ensureGraphAvailable) can. Also note that engine/CLAUDE.md says explicit locks must cover every file in getFileIds(). Here only getFileId() (the mutable) is locked, which is probably right because that is the file being rewritten, but a one-line comment saying why would help.

6. Writers are blocked for the whole read-and-rewrite phase. That includes the page scan, any document-scan fallback, and the page writes. It is a correctness tradeoff and the comment says so. For large indexes the timeout in item 1 will become the visible symptom (Cannot compact vector index ... timeout locking file). Consider logging at a level that makes the retry visible, and check that auto-scheduled compactions treat TimeoutException as "retry later" and not as a failure.

7. Commits blocked across the swap. A commit that was waiting on the old file lock proceeds after releaseCompactionFileLock(), with the old file id and index name now migrated. The existing setMigratedFileId and indexRenamed machinery should cover it, and the stress test exercises it probabilistically. A deterministic test (hold a commit at the lock, compact, release it) would be more convincing, but I understand it is hard to arrange.

8. LSM lookup cost for duplicate-heavy NOTUNIQUE indexes (valuePositionsOfRun). Every entry in the equal-key run, apart from mid, now has its key deserialized and measured, where before it was one getInt per entry. For a low-cardinality index (a boolean or status column with thousands of duplicates per key) that is an extra pass per lookup. Consider a fast path when all key types are fixed-width, or only re-measuring when an entry's size can differ (numeric variable-length types), since the correctness bug only exists for those.

Style / nits (per CLAUDE.md)

• Issue9071...Test.missing() uses the fully qualified com.arcadedb.database.Record. Import it instead.
• varIntView hard-codes 10. Name it like MAX_COMPRESSED_RID_SIZE, for example MAX_VARINT_SIZE.
• inferPropertyTypeFromExistingData now scans the entire type for every property on every CREATE INDEX, even when all values share one type, so large types pay a full extra pass. The Javadoc documents this as deliberate. Early exit on a mismatch is already there, which is good.
• CypherIndexMixedNumericIssue8999Test.firstColumn infers the column from the query text, which is fragile. Pass the column name explicitly.
• The Issue9034 test is 12,000 delete/insert cycles. It is tagged slow, which is right.

Positives

• Clamping the page view to the bytes left in the page is the right fix for NOTUNIQUE_HASH index: after hundreds of DELETE FROM and insert cycles, an equality lookup on one key throws IllegalArgumentException "newPosition > limit" until all the records are deleted #9034, and it is applied consistently across all five read helpers.
• Retaking the delta snapshot after the renumbering, and clearing the stale delta buffer at the swap, address the id-reuse hazard clearly. The comments explain why.
• The release in finally is idempotent and the "already held by this thread" case is handled.
• Every issue has a regression test.

🤖 Generated with Claude Code

Review round 2: fixed the unknown-class inference guard and the undeclared assertion. Not changed: (1) the data file commit lock window of an explicit COMPACT INDEX is the deliberate tradeoff, a timed-out compaction is retried and committers retry on lock timeout; narrowing it would need a delta merge under the lock. (2) STRING-keyed indexes built before this change hold "3.0"-style keys and need REBUILD INDEX; restoring the old spelling would bring back #8993.

Review

The root-cause analysis in the PR description is good and the fixes are targeted. I read the diff and the surrounding code but did not run the tests. Findings, most important first.

1. HashIndexBucket.insertIntoOverflow now compacts every full overflow page it walks (performance)

The new block runs compactPage(overflowPage, entryCount) whenever the first fit check fails. That includes the common case of a page that is full of live entries with no dead space. compactPage allocates a byte[entryCount][], copies every entry and rewrites it. That widens the page's modified range, so the WAL delta grows. A hot key on a NOTUNIQUE_HASH index (the boolean test is exactly this shape) walks the whole chain on every insert, so each insert pays this once per full page in the chain.

Suggest compacting only when the page has dead space, for example when readShort(BUCKET_DATA_END) - BUCKET_CONTENT_START is greater than the sum of the live entry sizes. The primary bucket path (~line 1630) already compacts only after a failed fit, but it is not walking a chain.

2. convertIndexKeyOrNull change applies to every String-typed index key, not only Cypher's fallback

Type.convertIndexKeyOrNull is shared by LSMTreeIndexAbstract.convertKeysToDeclaredTypes and HashIndex. The new early return fires for any index whose key type is STRING, including SQL indexes on a declared STRING property.

For such an index, stored values are already Strings and are not canonicalized. A lookup with a Double or BigDecimal parameter used to produce "3.0" and could match a stored "3.0". It now produces "3" and misses. That is a behaviour change outside the #8993 scenario.

Please add a regression test for a declared STRING property with an index queried with a double parameter. If it does regress, scope the canonicalization to the Cypher default-key-type path. Also, Type has no direct unit test for canonicalNumberKey. Cases worth covering: NaN, infinities, -0.0, 0.1f, a large-exponent BigDecimal, and 3/3.0/3.00.

3. Vector compaction holds the data file commit lock for the whole live-set read

lockDataFileForCompaction() waits up to COMMIT_LOCK_TIMEOUT (5 s by default) and then holds the lock from before the page read until after the rewrite and swap. For a large index, that read plus the rewrite can exceed 5 s, and concurrent commits to that index then fail with a lock timeout (NeedRetryException). The correctness trade is reasonable, since the lock is the only way to keep the live set equal to the file. Please confirm that:

• the TimeoutException thrown on acquisition is handled by the compaction callers as "retry later", as the Javadoc claims. I did not check the async and auto-compaction callers.
• the write-stall duration on a large index is acceptable, or document it.
• there is no lock-order inversion between graphBuildLock (held here before the file lock) and any commit path that takes the file lock first and then graphBuildLock. The comment near line 159 covers lock vs graphBuildLock, but the file-lock to graphBuildLock order should be checked too.

The new deltaSnapshotId = nextId.get() assumes nothing committed since the pages were read. That holds only if the lock was actually held. When lockDataFileForCompaction() returns -1 because the thread already held it, the holder is the caller, so it is probably fine, but a comment saying so would help.

4. Orphaned Javadoc in LSMTreeIndexAbstract

The Javadoc for valuePositionsOfRun sits above keySizeMayVaryAmongEqualKeys(), so two Javadoc blocks are stacked. The first one (about the run of equal keys) therefore documents the wrong method, and valuePositionsOfRun has none. Move the first block down to valuePositionsOfRun.

keySizeMayVaryAmongEqualKeys() also iterates binaryKeyTypes on every equal-key lookup. Caching it in a final boolean computed at construction would be cheaper, since lookups are a hot path.

5. Cypher type inference (#8999)

• The previous 256-record cap is gone for all properties, not only temporal ones, so every CREATE INDEX on a populated type now does a full extra scan per undeclared property. The Javadoc owns this, but it is a real cost on large types.
• In the catch (RuntimeException e) cleanup, type.dropProperty(declared) can itself throw and mask the original build failure. Wrap it, or use e.addSuppressed.
• Missing tests: none for the failed-build cleanup (a build that throws should leave no declared properties), and none for the FLOAT + DOUBLE -> DOUBLE widening or the BYTE/SHORT/INTEGER/LONG widening.

6. Tests

• Issue9071CompactionDuringConcurrentWritesTest sleeps a fixed 12 s and depends on compactions overlapping with writers, which is inherently probabilistic. It is tagged vector and asserts compactions > 0, which is good. Consider polling for a condition instead of a fixed sleep.
• The NOTUNIQUE_HASH index: after hundreds of DELETE FROM and insert cycles, an equality lookup on one key throws IllegalArgumentException "newPosition > limit" until all the records are deleted #9034 test (@Tag("slow"), 12k delete/insert cycles) is acceptable. A deterministic test that places an entry at the end of a page and calls the read path would be a more direct regression guard.
• The An LSM index on a FLOAT property: deleting the records that hold 0 and -0 fails at commit with NegativeArraySizeException and both records stay (new in main since #8968; DOUBLE is fine) #9033 tests look right.

Minor

• ridView/varIntView look correct and are a good fix. The extra Math.min per read is negligible.
• Style and CLAUDE.md conventions look followed (final params, no fully qualified names, AssertJ style). The comment blocks are long for this codebase, but that matches the neighbouring code.

Overall: the fixes look right for the described bugs. I would address #1 (perf) and #2 (possible regression) before merging, and confirm #3.

Review

The fixes look correct and well reasoned, and each issue has a targeted regression test. I read the diff and did not run the tests. Notes below, most important first.

Vector compaction lock (#9071)

• Lock hold time. lockDataFileForCompaction() is taken before buildGraphFromScratchExclusively and released only after rewriteDataFileWithLiveEntries. Between those points the build also runs the page read and, when pages are missing vectors, the document scan fallback (database.scanBucket(...)). That scan is linear in the type size. Writers on this index wait up to COMMIT_LOCK_TIMEOUT (5 s by default) and then fail with a timeout. Suggestions:
  • Confirm whether that fallback runs inside the locked span. If it does, either skip the compaction when it triggers or document the trade-off.
  • Add a comment near compactionFileLock saying the commit-blocking window is "page read + optional scan + rewrite".
• Single file only. engine/CLAUDE.md says explicit locks must cover every index component via getFileIds() (plural). The Javadoc justifies locking only the mutable data file because the compacted sub-index is read-only. I agree for commits, but a reviewer who knows the commit-time lock-coverage check should confirm that locking the mutable alone is enough, since a miss there throws a non-retryable TransactionException.
• Delta reset. deltaVectors = new ArrayList<>() is safe only if it runs in the same critical section as the swap. It sits next to publishLocationIndex, so I believe it does, and deltaSnapshotById is built after the rewrite. A one-line comment saying so would help.
• Test. Issue9071CompactionDuringConcurrentWritesTest uses a fixed Thread.sleep(12_000). That is acceptable under @Tag("vector"). The compactions > 0 assertion could flake if every COMPACT INDEX times out against four busy writers. Consider raising COMMIT_LOCK_TIMEOUT for this database, or looping until at least one compaction succeeds instead of running for a fixed time.

HashIndexBucket (#9034)

• The ridView/varIntView clamp to getMaxContentSize() - offset is correct and cheap.
• compactPage now resets BUCKET_DATA_END when entryCount == 0, and insertIntoOverflow compacts before declaring the page full. Both are sensible. hasDeadSpace is O(entries) but runs only on the "does not fit" path, so there is no hot-path cost.
• Consider a direct test for the emptied-page case (the follow-up noted in the compactPage comment). Issue9034...Test covers it only indirectly through delete/insert cycles.

LSM lookup (#9033)

• keySizeMayVaryAmongEqualKeys() iterates binaryKeyTypes on every lookup that finds a run. Caching it in a final field computed at construction would avoid the repeated scan.
• Correct for FLOAT, DOUBLE and DECIMAL. Please check whether any other key type can compare equal with a different serialized size (for example a case-insensitive string). If none can, the Javadoc is accurate as written.

Canonical number keys (#8993)

• canonicalNumberKey does (Double) number in the non-Float branch. That is only safe because the caller checks instanceof Double || Float || BigDecimal, so the contract is implicit. Handling Double explicitly (or guarding with an IllegalArgumentException) would make it robust to other callers.
• isUndeclaredProperty does a schema lookup per key conversion, but only for Double, Float or BigDecimal keys on a STRING key type. That is acceptable. It also means keys written before and after a later property declaration can differ in spelling. Please confirm that case is covered or intentionally out of scope.

Cypher type inference (#8999)

• Dropping the 256-record cap means CREATE INDEX does a full scan on every untyped property. The Javadoc says this is deliberate. It is a real cost on large types, so please mention it in the release notes.
• The rollback on a failed build drops only the properties created in this statement (declaredHere), which is correct, and a failing drop is kept as a suppressed exception.
• integralRank uses default -> 3 for LONG. Fine today, but an explicit LONG case with a throwing default would be safer if isIntegral ever grows.
• A temporal value after a non-temporal one returns null, while a non-temporal value after a temporal one is skipped. The comment explains the asymmetry. A test pinning both orders would help if CypherIndexMixedNumericIssue8999Test does not already have one.

Style and conventions

• Mostly in line with CLAUDE.md: tests use assertThat, final is used, and I saw no stray System.out.
• The Javadoc on Type.canonicalNumberKey has a line well over the 160-column width.
• Several comments are long multi-paragraph blocks. That matches the surrounding files, but the repo guidance prefers short comments.

No blocking issues from my read. The main item to resolve is how long writers are held off in the compaction path (first bullet).

Review of #9132

The fixes are well targeted, each has a regression test, and the comments explain the why. I read the main-code diff in full and skimmed the tests. I did not run anything. Findings below, most important first.

Worth a look

1. Vector compaction (LSMVectorIndex): commit stall while the file lock is held. lockDataFileForCompaction() takes the data file commit lock before the pages are read and keeps it until the rewrite finishes. That span includes the document-scan fallback that runs when pages miss vectors, and the scan can be long on a big type. Writers wait up to COMMIT_LOCK_TIMEOUT and then fail with a TimeoutException. This is the right trade for correctness, but please state the expected worst case in the javadoc. A test with the fallback path active (pages missing vectors) would also help, because the new test only exercises the happy path.

   • Other builders of the same index queue on graphBuildLock behind a compaction that is itself waiting up to COMMIT_LOCK_TIMEOUT for the file lock. I found no inversion. Commits take the file lock and then the index lock, and graphBuildLock is taken first and never after the index lock, so the ordering holds. It is still the most delicate part of the PR and deserves a second pair of eyes.
   • compactionFileLock is documented as "only touched under graphBuildLock", yet it is released from buildGraphFromScratchExclusively. It is correct today because buildGraphFromScratchWithRetry holds graphBuildLock for the whole build. Making the field non-volatile and relying on that is fragile if the build is ever moved to another thread. unlockFile is also bound to Thread.currentThread().

2. inferPropertyTypeFromExistingData: full scan on every CREATE INDEX over an undeclared property. The old 256-record cap is gone for all types, not only temporal ones. Correctness wins here and the javadoc says so, but it is a real cost on huge types. It stays a single sequential scan with early exit on the first disagreeing value, which is acceptable. Consider mentioning it in the release notes.

   • FLOAT + DOUBLE settles on DOUBLE. A float such as 0.1f then widens to 0.10000000149011612 in the index key, while a lookup with the double 0.1 will not match it. Type.canonicalNumberKey deliberately uses widenFloat to avoid exactly this on the STRING path. It is an edge case, but returning null (undeclared) for that pair would be consistent with the other mixed-type decisions.
   • In the catch-all catch (final Exception ignored), a failure halfway through the scan returns null and silently hides it. This is pre-existing, but now that the scan is longer, a LogManager fine-level message would make it debuggable.

Minor

3. LSMTreeIndexAbstract.valuePositionsOfRun: keySizeMayVaryAmongEqualKeys() loops over binaryKeyTypes on every equal-key lookup, which is a hot path. binaryKeyTypes is fixed per index, so compute it once in a final field.

   • The method moves currentPageBuffer.position() for entries other than mid, where before the loop only used absolute getInt. Both callers appear to reposition before reading, but please confirm. A short comment, or restoring the position, would make that safe.
   • The measuring path is only exercised for FLOAT zero. A test with DOUBLE or DECIMAL equal-but-different-size keys (1.5 vs 1.50) would cover the other types the guard names.

4. HashIndexBucket:

   • compactPage(...) == entryCount is always true, so that part of the condition is dead. Calling compactPage(...) and then checking freeSpace is clearer.
   • hasDeadSpace walks every slot before every compaction. That is acceptable because it only runs when the entry does not fit, and it saves an unnecessary page rewrite.
   • ridView and varIntView clamp against page.getMaxContentSize() - offset. If an offset can ever exceed the content size this becomes a negative length. That should be impossible for valid pages, but a Math.max(0, ...) would turn a corrupt page into a clean read error instead of an odd exception.

5. executeCreateIndex rollback: it only catches RuntimeException and silently suppresses drop failures. That is fine. A property that already had data and was dropped on failure leaves values in the records, as expected. Adding a test for the ignoreIfExists path would pin that the rollback only touches properties declared by this statement.

6. Tests:

   • Issue9071CompactionDuringConcurrentWritesTest uses a fixed Thread.sleep(12_000) and a 60 s deadline. It is tagged vector, which is correct. It does not assert wall-clock elapsed time, so it follows the CLAUDE.md rule. The catch (TimeoutException | IndexException) swallows retry cases, and the compactions > 0 assertion guards against a vacuous pass. Good.
   • Issue9034HashLookupAfterDeleteInsertCyclesTest runs 12,000 delete/insert cycles. It is tagged slow correctly. It would be worth adding a deterministic unit-level test of the page-end read, because the random-seed repro depends on page layout.
   • The new tests follow the assertThat(...).isX() style.

Security / dependencies

No new dependencies, no new input surface, and no security concerns. Type.canonicalNumberKey caps the plain-string expansion at an exponent of 400, so a huge BigDecimal cannot allocate a giant key string.

Overall this looks good to me. Points 1 and 2 (the FLOAT/DOUBLE widening) are the ones I would address or explicitly accept before merge.

Review: well-reasoned set of fixes with a regression test per issue and comments that explain the why. No blockers found. Notes below are mostly about behavior and risk rather than defects.

Vector compaction (#9071), LSMVectorIndex

• Taking the data-file commit lock before the page read, retaking deltaSnapshotId after the renumbering, and clearing the stale deltaVectors at the swap all look right. The lock-order argument (file lock, then index lock; graphBuildLock never taken by a commit holding a file lock) is documented and consistent with the existing REBUILD_SEMAPHORE ordering comment.
• Writer stall window: the lock now covers the page read plus the document-scan fallback, not just the swap. On a large index this can block every writer to that index for the whole span, and writers time out after COMMIT_LOCK_TIMEOUT. Correctness requires it, but a log line with the held duration would make a "commit timeout during compaction" report easy to diagnose.
• The compaction thread can wait up to COMMIT_LOCK_TIMEOUT for the lock while holding graphBuildLock, which stalls other builders/searches queued on it. The TimeoutException retry-later path in compact() handles it, but keep it in mind.
• compactionFileLock is a plain field guarded by "only touched under graphBuildLock". Both release sites run on the same thread under that lock, so it is fine; an assertion (graphBuildLock.isHeldByCurrentThread()) would make the invariant enforceable.

Hash bucket (#9034), HashIndexBucket

• ridView/varIntView clamping with getMaxContentSize() - offset is correct. Consider guarding against a negative length if offset ever exceeds the content size on a corrupted page, so the failure is a clear exception.
• hasDeadSpace walks every slot and compactPage walks them again; only on the "entry does not fit" path, so acceptable.
• The entryCount == 0 branch of compactPage now resets BUCKET_DATA_END. Good catch.

LSM equal-key run (#9033)

• valuePositionsOfRun only measures per-entry key size when a key type is FLOAT/DOUBLE/DECIMAL, so the common path keeps its cost. It moves currentPageBuffer.position(...); I could not see a caller relying on the buffer position afterwards (two call sites), but worth a double check.

Cypher type inference (#8999) and canonical keys (#8993)

• Removing the 256-record cap makes CREATE INDEX do a full extra scan of the type per undeclared property. The Javadoc says this is deliberate and I agree correctness wins, but it is a DDL cost increase on very large types.
• Mixed int/float now leaves the property undeclared and falls back to STRING keys, so range predicates on such properties follow string ordering. That is the stated trade-off versus silent narrowing, but it deserves a line in the docs/release notes.
• isUndeclaredProperty does a schema lookup per key conversion, but only when a Float/Double/BigDecimal meets a STRING key type, so the hot path is untouched.
• canonicalNumberKey (BigDecimal.valueOf(double) + stripTrailingZeros().toPlainString(), -0.0 maps to "0", 400-digit guard) looks correct.
• Dropping the properties declared by a failed CREATE INDEX is a good fix.

Tests

• Each fix has a dedicated IssueNNNN...Test, matching the regression-test rule. The LSM_VECTOR: records written while COMPACT INDEX runs are left out of the vector index for good, so a search with a record's own vector does not return it (before and after close, until REBUILD INDEX; also on 26.9.1) #9071 concurrent test is the most likely to be timing sensitive: please confirm it is tagged @Tag("vector") or @Tag("slow") as appropriate and asserts no raw wall-clock bounds (use StallAwareStopwatch if any).
• A direct test for the new hasDeadSpace branch in insertIntoOverflow (non-empty overflow page with holes) would be nice if the NOTUNIQUE_HASH index: after hundreds of DELETE FROM and insert cycles, an equality lookup on one key throws IllegalArgumentException "newPosition > limit" until all the records are deleted #9034 test does not already reach it.

Style

• Conforms to CLAUDE.md (imports, final, braceless single-statement ifs). Some new Javadocs are long, but they explain subtle invariants.

Looks good to me pending the test-tagging confirmation above.

Codecov Report

❌ Patch coverage is 75.80645% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.57%. Comparing base (54fecd1) to head (0bba687).
⚠️ Report is 37 commits behind head on main.

@@             Coverage Diff              @@
##               main    #9132      +/-   ##
============================================
- Coverage     76.57%   76.57%   -0.01%     
- Complexity     3904     3905       +1     
============================================
  Files          2116     2116              
  Lines        209840   209940     +100     
  Branches      44367    44393      +26     
============================================
+ Hits         160695   160769      +74     
- Misses        31888    31907      +19     
- Partials      17257    17264       +7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.