feat(#4085): split a sparse-vector top-K into parallel RID ranges

[lvca]

Closes #4085.

Wires the dispatch SparseVectorScoringPool was built for: a top-K over a single LSM_SPARSE_VECTOR index is scored as several RID ranges concurrently and merged, instead of one traversal on the caller's thread.

Measured

SPLADE-shaped corpus, INT8, 18-worker box. Single query, ranges forced:

corpus	serial	8 ranges	16 ranges
500k	13.4 ms p50	4.0 (3.4x)	2.4 (5.6x)
1M	26.2 ms p50	7.6 (3.5x)	4.5 (5.9x)

LSMSparseVectorIndexLargeBenchmark, which now runs both shapes and asserts they return the same documents:

corpus	serial	adaptive split
1M	5.13 ms/query	2.83	1.82x
10M	24.18 ms/query	6.90	3.51x

The 1M gain is smaller there because that benchmark's query is 10 terms over 30 nnz/doc: at 5 ms of work, per-range cursor setup and the merge take a visible share. The split pays in proportion to how much work the query does.

Why the gating is most of the diff

A range prunes against its own top-K watermark rather than the global one, so it does more work than its share: 1.16x total CPU at 2 ranges, 1.89x at 8. That is free latency on an idle machine and stolen throughput on a busy one, so the decision has to be adaptive.

Sizing it from pool activity was tried first and measured wrong. A query runs on its caller's thread, so at 16 concurrent clients the pool reads idle at every sampling instant: a third of queries split anyway and throughput fell 14%. The gate that works counts queries in flight, not pool activity. At 500k:

clients	serial QPS	adaptive QPS	p50	queries split
1	72	322	3.05 vs 13.7 ms	100%
4	290	468 (+61%)	4.13 vs 13.6 ms	74%
16	869	829 (-4.6%)	19.0 vs 17.8 ms	2 of 8289

A query also refuses to split when it is already running on a pool worker (a nested fan-out on a bounded queue deadlocks rather than degrading, since the outer tasks hold every worker while waiting on inner tasks nothing is left to run), and when the caller holds uncommitted page changes, which a worker's own transaction context cannot see.

Results are the serial ones, ties included

RidScoreMinHeap now breaks ties on RID ascending, so which of several equally-scored documents survives no longer depends on heap layout, and the merge ranks the same way. This is a behaviour change: a query with tied scores could previously return either document and now deterministically returns the lowest RID.

Scores can differ by one ulp between the two shapes, because MaxScore sums a document's terms in an order that follows the pruning split. Document set and ranking are unaffected; documented on the test that asserts it.

Also here

• The caller scores one range itself rather than blocking on all of them.
• topKGrouped stays serial: merging grouped results needs the per-group worsts, not a flat best-k.
• Studio's Executor Pools card gains three columns on the sparse-vector row (reserved workers, queries in flight, queries split). Without them an idle row is ambiguous between "nobody querying", "load gate switched splitting off", "queries too small" and "broken".
• MultiBucketSealedSegmentFanoutTest covers multi-bucket over sealed segments, which had no coverage and passes today only because LocalDatabase.checkDatabaseIsOpen creates a thread context as a side effect. Nothing states that contract, so a refactor could have broken segment-backed multi-bucket queries silently.

New settings

• arcadedb.sparseVectorScoringMaxPartitions (0 = adaptive, 1 = off, >1 = explicit and bypasses the load gate)
• arcadedb.sparseVectorScoringMinPostingsForPartitioning (default 200,000)

Testing

Full engine module 10,128 tests and full server module 763 tests, zero failures. New ParallelRangeTopKTest covers range/serial equivalence at 2..16 ranges, tie determinism, empty and out-of-bounds ranges, engine-level equivalence, no nested split, no worker leak, and in-flight suppression.

Not verified: the Studio card rendering in a browser. The gauges and the JSON contract are pinned by PoolMetricsTest, but nobody has looked at the page.

Worth a reviewer's attention

1. The load gate is a heuristic. It measured well at 1/4/16 clients on one machine and is the piece most likely to want tuning on other hardware.
2. The tie-break change is user-visible for tied scores.
3. Whether explicit maxPartitions > 1 should really bypass the load gate, or whether an operator asking for N ranges should still yield under load.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XF4M78mPjg8VH7GR5pPZEp

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 high · 7 minor

Alerts:
⚠ 8 issues (≤ 0 issues of at least minor severity)

Results:
8 new issues

Category	Results
ErrorProne	1 high
CodeStyle	7 minor

View in Codacy

🟢 Metrics 12 complexity

Metric	Results
Complexity	12

View in Codacy

🟢 Coverage 85.31% diff coverage · -6.61% coverage variation

Metric	Results
Coverage variation	✅ -6.61% coverage variation
Diff coverage	✅ 85.31% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (c906c6b)	146708	108811	74.17%
Head commit (013468d)	178902 (+32194)	120869 (+12058)	67.56% (-6.61%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#5518)	211	180	85.31%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{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.}

[claude]

Review: parallel RID-range top-K (#4085)

Went through this end to end. It is a strong, unusually well-documented change: the reservation-based load gate is a genuinely clever answer to the "a query runs on the caller thread, so the pool always looks idle" blind spot, the tie-break change makes serial/parallel equivalence provable rather than hand-waved, and the test suite covers the parts that matter (range equivalence at 2..16, tie determinism, empty/out-of-bounds ranges, no-nest, worker release, in-flight suppression, engine-level split, plus the large-benchmark equivalence assertion). The reasoning captured in the Javadoc is the kind that stops a future refactor from silently regressing this.

A few things worth a second look before merge, none of them blockers:

1. Reservation is acquired and released in different methods

planPartitionBoundaries() calls reserveWorkers / tryReserveWorkers (PaginatedSparseVectorEngine.java:511/514) and returns; the matching releaseWorkers lives in topK()'s finally (line 381). Today that is safe only because everything between the reservation and the return boundaries is pure in-memory array access (blockFirstBucketId/Position). But the method is declared throws IOException, so if any future edit adds a page read while building the boundary array, an exception there leaks the reservation permanently, silently shrinking what every later query believes the pool has free. That is exactly the failure mode claimedWorkersAreAlwaysHandedBack warns about, and the split ownership makes it hard to keep covered. Consider doing the reservation in topK immediately around the parallel block, or wrapping the tail of planPartitionBoundaries so a throw after reserving releases before rethrowing.

2. inFlightQueries double-counts the multi-bucket fan-out

The per-bucket fan-out in SQLFunctionVectorSparseNeighbors submits each bucket's idx.topK(...) to the pool, and that path reaches engine.topK (LSMSparseVectorIndex.java:324), which calls pool.queryStarted() unconditionally. So a single 8-bucket user query registers 8 in-flight "queries", all on pool workers. Those workers correctly refuse to split (isPoolThread), but they inflate inFlightQueries, and the load gate inFlightQueries * 2 > ceiling trips at ceiling/2. One multi-bucket query can therefore suppress splitting for an unrelated concurrent single-bucket query even on an otherwise idle box. Since the whole point of counting queries rather than tasks is to see caller-thread load, counting pool-thread queries in the gate works against that intent. Cheapest fix: skip queryStarted/queryFinished (or at least the gate contribution) when isPoolThread().

3. Explicit maxPartitions > 1 has no clamp to pool capacity

The auto path clamps wanted to getMaxParallelism(); the explicit path only clamps to byLayout (line 510). On a large index an operator setting, say, maxPartitions=1000 gets 1000 ranges, i.e. 999 tasks and 1000 cursor stacks, massively oversubscribing an 18-thread pool. The caller-runs policy keeps it from deadlocking, but the memory/overhead is real and unbounded. This overlaps your own open question #3. Even if explicit means "opt out of throttling", a sanity clamp (e.g. to a small multiple of pool size) or an explicit note in the setting doc would be worth it.

4. The tie-break change affects serial-only users too

Worth emphasising in the release notes beyond "results are the serial ones": because RidScoreMinHeap and TopKCollector.drain() both now rank on RID ascending, the deterministic tie ordering applies even with maxPartitions=1 (splitting fully disabled). Any user with tied scores gets a potentially different retained set than before, regardless of whether they ever enable this feature. The new order is the better one (deterministic), but it is not opt-in.

Nits

• MultiBucketSealedSegmentFanoutTest uses the fully-qualified com.arcadedb.index.sparsevector.SparseVectorScoringPool twice (lines 124, 152); CLAUDE.md asks for an import instead.
• No direct test of the timeout / failed-range paths in parallelTopK (the IndexException on timeout, addSuppressed on a failed sibling). The happy path and cancellation-on-interrupt are implicitly exercised but the deadline branch is not.

Overall this looks careful and correct, and the correctness claim is well pinned by tests. Points 1 and 2 are the ones I would most like to see addressed.

[lvca]

Thanks, this was a useful review. Pushed as 7bf17702a. Three of the four taken as written, one fixed differently, and fixing it turned up a regression worth recording.

1. Reservation acquired and released in different methods. Agreed, and fixed. The claim is now released on any throw between acquiring it and returning the boundaries. You are right that the walk cannot currently throw, but the method reads segment metadata and is declared throws IOException, so the next edit that adds a page read there would have leaked a claim permanently with nothing failing to point at it.

2. inFlightQueries double-counts the multi-bucket fan-out. The bug is real, the suggested fix would have introduced the opposite one. Those eight per-bucket queries are genuine load, so making them invisible would leave a split free to oversubscribe against them. Instead the counter is now split in two: caller-thread queries drive the load gate, pool-thread queries subtract from grantable capacity. Each measures the thing it is good at.

Worth flagging what that cost me. My first implementation read ThreadPoolExecutor.getActiveCount() for the second half, which looks like the obvious source and is a trap: it takes the pool's main lock and walks the worker set. The benchmark caught it immediately - at 16 concurrent clients throughput fell to 549 qps against 772 serial while only 2 of 5499 queries split. Splitting was off, so the entire loss was in the decision path: a decision more expensive than the decision it was making. A plain AtomicInteger put it back to 718 against 709. It also retroactively explains an "activity-aware" measurement earlier in this work that I had written off as noise. The trap is now documented at the call site so nobody re-adds it.

3. Explicit maxPartitions has no clamp. Agreed, now capped at 4x pool size. The setting means "do not throttle me", not "open a thousand cursor stacks"; past a small multiple of the pool there is no thread free to run them anyway.

4. Tie-break affects serial-only users. Correct, and the more important half of that point. Now documented where the behaviour lives: the ordering is in the collector, so a database with splitting disabled entirely gets it too, and anyone whose scores tie can see a different retained set than a previous release.

Nits. Import fixed. The deadline path is now tested, and deterministically rather than by racing a clock: every worker is jammed with a latch so the range gets submitted but never started, and the test also asserts the timed-out query hands its claim back.

Verification, including what I could not finish

• Sparse-vector and vector suites: 403 tests green
• Server module, in full: 768 tests green
• Throughput re-measured after the reservation change: 1 client 3.80 ms p50 vs 14.4 serial, 4 clients +67% throughput at 4.74 ms vs 16.5, 16 clients at parity (718 vs 709 qps) with 2 of 7184 split

The full engine module did not complete. Three attempts were killed by SIGTERM (exit 143) on three different unrelated classes - TestSortedIndexBuildBenchmark, GAVEligibilityTest, ACIDTransactionTest - with no hs_err files, so an external process killer on this machine rather than a JVM crash. The best of those runs reached 6765 tests with zero failures before being cut off, and the same command completed 10,128 green on the parent commit.

What bounds the risk: every production file in this commit is inside com.arcadedb.index.sparsevector, and RidScoreMinHeap is package-private with exactly one consumer in the same package. That package's suite is the 403 green above. Still worth a clean full-module run in CI before merge, and I would not claim otherwise.

[claude]

Review: split a sparse-vector top-K into parallel RID ranges (#4085)

Thorough, well-instrumented change. The load-gate reasoning, the reservation-vs-observation argument, and the tie-break determinism are all carefully documented, and the test coverage (range/serial equivalence, tie determinism, no-nested-split, worker-leak, in-flight suppression, deadline release) is excellent. One issue below is worth blocking on; the rest are minor.

1. (High) parallelTopK can roll back the caller's transaction under the pool's caller-runs policy

In PaginatedSparseVectorEngine.parallelTopK, each worker task calls DatabaseContext.INSTANCE.init(database) unconditionally, then removeContext(...) in a finally. That is safe only on a fresh worker thread with no context. But SparseVectorScoringPool uses a bounded LinkedBlockingQueue plus a CallerRuns rejection policy, so when the queue is full a submitted range task runs inline on the caller thread (task.run() in the pool's rejection handler).

The caller thread already holds an active transaction context, so init() takes the else branch in DatabaseContext.init and hits the "ROLLBACK PREVIOUS TXS" loop (DatabaseContext.java:96-107): it rolls back the caller's in-progress transaction, then the finally removeContext(...) wipes the context entirely. After that inline task returns, the caller continues to score its own range 0 (and the outer query/transaction proceeds) with no context, failing with Transaction context not found on current thread - and the user's transaction has already been silently rolled back.

Why this is reachable:

• Explicit sparseVectorScoringMaxPartitions > 1 bypasses the reservation gate (reserveWorkers is unconditional, with no free-capacity check), so on a busy server the submitted ranges can overflow the bounded queue.
• Even in adaptive mode there is a window: tryReserveWorkers counts poolThreadQueries (tasks that have started), not tasks sitting in the queue, so a burst of concurrent per-bucket fan-outs can fill the queue while capacity still looks grantable.

Contrast with the two patterns this copies:

• LocalDatabase.checkDatabaseIsOpen guards it: if (getContextIfExists(databasePath) == null) init(this); (LocalDatabase.java:2563).
• FetchFromTypeExecutionStep's identical init/removeContext pair is safe only because ParallelScanProducerPool uses an unbounded queue and the default abort policy, so its tasks never run on the caller thread.

Suggested fix - mirror checkDatabaseIsOpen, only establish (and only tear down) a context this task actually created:

final boolean created = DatabaseContext.INSTANCE.getContextIfExists(database.getDatabasePath()) == null;
if (created)
  DatabaseContext.INSTANCE.init(database);
try {
  return rangeTopK(...);
} finally {
  if (created)
    DatabaseContext.INSTANCE.removeContext(database.getDatabasePath());
}

(or skip init/remove entirely when !SparseVectorScoringPool.isPoolThread()). A regression test that fills the queue and forces a range task through caller-runs while the caller is inside an active transaction would pin this down - the existing aRangeThatNeverGetsAWorkerFailsTheQueryOnTheDeadline jams workers but does not exercise the caller-runs branch.

2. (Minor) Shared deadline is consumed by the caller's own range-0 scoring

parallelTopK computes deadlineNs and then scores range 0 synchronously before draining the worker futures. If range 0 legitimately consumes the whole deadline, the subsequent f.get(remainingNs, ...) can report a TimeoutException and fail the query even though the worker ranges already completed. Balanced ranges make this unlikely, but consider draining the futures concurrently with range 0, or basing the timer on submission time.

3. Notes (no action needed)

• The RidScoreMinHeap tie-break (score desc, RID asc) and mergeRanges are consistent, and the merge is correct for tied scores at the k-th boundary (any range that drops a tied doc already holds k entries ranking above it, so the doc is globally excluded too). The global behaviour change for tied scores is clearly called out in the PR body and Javadoc.
• Reservation release is well covered: both the explicit and adaptive paths release exactly what they reserved, including the boundary-walk catch and the topK finally, and claimedWorkersAreAlwaysHandedBack / the deadline test assert no leak.
• Studio: 9 <th> match the new colspan=9, and the gaugeOrDash "absent != zero" handling is correct; PoolMetricsTest pins the JSON contract.

Overall a strong change; #1 is the one I would want addressed before merge.

Reviewed by Claude Code.

[lvca]

Good catch on #1, and it is worse than a crash. Fixed in 8cb6731b8.

1. Caller-runs rolling back the caller's transaction. Confirmed exactly as described. DatabaseContext.init on a thread that already holds a context takes the "ROLLBACK PREVIOUS TXS" branch and rolls back every transaction on it, and my finally removeContext then wiped the context the rest of the query still needed. So the visible failure is Transaction context not found on current thread, sitting on top of a user transaction that has already been silently rolled back - the quiet half being the dangerous one.

Fixed with the create-only-if-absent guard you suggested, tearing down only what the task created. Your diagnosis of why the pattern was unsafe here is the part I had wrong when I wrote it: I copied FetchFromTypeExecutionStep's init/remove pair without noticing that it is safe only because ParallelScanProducerPool has an unbounded queue and therefore never runs a task on the caller. That distinction is now recorded at the call site so the next person copying it sees the constraint.

The new test drives it deterministically rather than hoping to hit the window: it saturates every worker and every queue slot so the range is rejected onto the caller, with the caller inside an open transaction, and asserts both the result and that the transaction and its context are still alive afterwards. Verified it fails without the fix with precisely the reported symptom, so it is a real regression test and not a decoration.

You were also right that aRangeThatNeverGetsAWorkerFailsTheQueryOnTheDeadline does not reach this branch: it jams workers but leaves queue capacity, so submission succeeds and nothing runs inline.

2. Shared deadline consumed by the caller's own range 0. Agreed, fixed. A future that is already done is now harvested regardless of the deadline; the timeout governs waiting, not collecting. Failing a query while every worker range sits complete and waiting to be read was the wrong outcome, however unlikely balanced ranges make it.

3. Noted, thanks for checking the merge behaviour at the k-th boundary and the Studio colspan - both were on my list of things I believed but had not proven.

Verification

• ParallelRangeTopKTest: 10 tests green, including the new caller-runs case
• Sparse-vector and vector suites: 404 green
• Full server module was green on the previous commit; this commit touches only PaginatedSparseVectorEngine and one test

Still outstanding from my last comment: the full engine module has not completed on this machine - three runs killed by SIGTERM on unrelated classes, best reaching 6765 tests with zero failures. Worth a clean CI run before merge.

[claude]

Review: split a sparse-vector top-K into parallel RID ranges (issue 4085)

I read through the engine changes (BmwScorer, PaginatedSparseVectorEngine, RidScoreMinHeap, SparseVectorScoringPool), the server/Studio metrics, and the new tests. This is a strong, unusually careful piece of work: the correctness argument (disjoint RID partition, deterministic tie-break, merge = serial result) holds up, and the concurrency hazards are handled rather than hoped away. A few things I checked closely and found already correct, plus some minor observations.

Verified correct (worth calling out)

• Caller-runs context safety. The bounded queue + CallerRunsPolicy means a submitted range can execute inline on the users own thread inside their transaction. The contextCreated guard (PaginatedSparseVectorEngine ~L599-607) correctly creates-only-if-absent and tears down only what it created, so it never hits DatabaseContext.init ROLLBACK-PREVIOUS-TXS branch on the caller. ParallelRangeTopKTest.aRangeForcedOntoTheCallerThreadLeavesTheCallersTransactionAlone pins exactly this. Good catch and good test.
• No nested-fan-out deadlock. isPoolThread() gating in planPartitionBoundaries plus leaf-only worker tasks (workers never submit) means the pool always drains. Solid.
• Reservation accounting is symmetric on every exit path (release in finally, and the boundary-walk try/catch releasing on a future throw), and the false-timeout-on-a-completed-future case is defused by the f.isDone() harvest (~L631).
• Tie determinism. RidScoreMinHeap.compareEntry (highest-RID-first eviction) + strict > admission on ascending-RID DAAT + BY_SCORE_DESC merge = lowest-RID-wins, matching serial at the k-boundary. The behaviour change is real but the new answer is the stable one, and it is documented.

Minor observations

1. Test tagging. ParallelRangeTopKTest builds a 6,000-doc x 24-dim corpus across ~10 methods on the shared caller thread. If wall time is more than a couple of seconds, CLAUDE.md asks for @tag("slow") (the benchmark is already @tag("benchmark")). Worth checking its runtime and tagging if it is noticeably slow.
2. queries.split vs. effective parallelism. The split counter is incremented at the decision point in topK, before the pool decides whether tasks run on workers or inline via caller-runs. Under saturation an operator can see a high queries.split while the work actually ran serially inline (visible only by cross-referencing callerRunFallbacks). Might be worth a one-line note on the Studio card or the metric description so the two are read together.
3. Balance heuristic with multiple live segments. planPartitionBoundaries derives cut RIDs from the single widest dim in the widest segment. Correctness is unaffected (the ranges still cover RID space disjointly), but when several unflushed segments coexist before compaction, one segments block layout may not represent the global RID distribution and ranges can go lopsided. Fine as a heuristic; flagging in case it shows up as uneven per-range latency on freshly-loaded indexes.

On the three items you flagged for reviewers

• Load gate (inFlightQueries * 2 > ceiling). The reasoning for counting queries-in-flight over pool activity is convincing and the measurements back it. Agreed it is the piece most likely to want per-hardware tuning; keeping it behind a config knob is the right call. No change requested.
• Tie-break user-visibility. Documented on the asserting test and in the heap Javadoc. Fine to ship; may deserve a line in release notes since it can change which tied document is returned across a version bump.
• Explicit maxPartitions > 1 bypassing the load gate. My inclination: keep the bypass (operator opt-in is a deliberate do-not-throttle-me), but the clamp to maxParallelism * EXPLICIT_PARTITION_OVERSUBSCRIPTION is what keeps it from being a foot-gun, so that clamp is doing real work and should stay. If anything, a WARNING-throttled log the first time an explicit split is granted under high in-flight load would give operators the signal without changing behaviour.

Nice work overall. The gating rationale in the commit message and Javadoc is genuinely useful for the next person who touches this path.

[codecov]

Codecov Report

❌ Patch coverage is 77.96610% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.41%. Comparing base (c906c6b) to head (dcf2aee).
⚠️ Report is 58 commits behind head on main.

Files with missing lines	Patch %	Lines
...ndex/sparsevector/PaginatedSparseVectorEngine.java	73.98%	18 Missing and 14 partials ⚠️
...db/index/sparsevector/SparseVectorScoringPool.java	71.64%	13 Missing and 6 partials ⚠️
...ava/com/arcadedb/index/sparsevector/BmwScorer.java	95.65%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5518      +/-   ##
============================================
+ Coverage     65.31%   65.41%   +0.10%     
+ Complexity     1114      971     -143     
============================================
  Files          1748     1748              
  Lines        146708   147233     +525     
  Branches      31358    31468     +110     
============================================
+ Hits          95817    96316     +499     
+ Misses        37940    37662     -278     
- Partials      12951    13255     +304     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[lvca]

Thanks. Review points addressed in 2ec078f8d, and CI then caught something worse than any of them, fixed in 013468d80.

CI: my test hung the unit-tests job for an hour

unit-tests failed at 1h0m14s, and the thread dumps name
aRangeForcedOntoTheCallerThreadLeavesTheCallersTransactionAlone - the caller-runs test from
the last round. The irony is exact: it deadlocked on the very mechanism it was written to
test.

It saturated the pool by computing "workers + free queue slots" from a snapshot and then
submitting that many blocking tasks. The snapshot races, and overshooting by one is not a
slow test but a permanently stuck one - the surplus task is rejected, caller-runs executes it
on the submitting thread, and it waits on a latch released only in a finally that thread can
no longer reach. Locally the arithmetic happened to come out right, which is why it passed
every run here and hung on the first CI runner with a different core count.

Fixed twice over, either half being sufficient: submit until the queue reports itself full
rather than computing a count, and have a saturation task block only when it finds itself on a
worker, with a timeout rather than indefinitely. A mis-sized saturation now fails the test
instead of wedging the job, which is the difference between a diagnosis and an hour of
nothing. The same shape was in the deadline test's hogs, so it got the same treatment.

I re-verified the test still earns its place rather than being quietly defanged: with the
production contextCreated guard removed it still fails with "Transaction context not found
on current thread", so the rewritten loop still saturates for real.

The other two CI failures are not mine

• integration-tests: Issue4141SessionManagementIT gets HTTP 400 from
  /api/v1/command/graph. The same job fails identically on test(engine) #5519: isolate what a tag-heavy TimeSeries schema pays on ingest #5520, an unrelated PR that has
  already merged, and nothing in this branch touches HTTP, sessions or command handling.
• ha-integration-tests: failed too; I have not diagnosed it yet and will not claim it is
  unrelated until I have.

Review points

1. Test tagging. Not taken, with a measurement: the class runs 10 tests in 1.46 s, which is
not what CLAUDE.md means by noticeably long, and @Tag("slow") would pull a
transaction-safety regression test out of regular CI. (It is 3.5 s after the saturation fix,
still under the bar.)

2. queries.split vs effective parallelism. Good point, taken. It counts the decision, not
the outcome, and under caller-runs a query counted as split can have run serially. Now stated
in the metric description and on the Studio card, next to the Caller-Run Fallbacks column
where it shows up.

3. Balance heuristic with multiple live segments. Taken as documentation. Boundaries come
from one dim of one segment, so before compaction merges several live segments that layout
need not represent the global RID distribution and ranges can come out uneven. Correctness is
unaffected - the ranges still partition the RID space and every document is scored exactly
once - only the speedup, and it shows up as one range finishing after its siblings.

On the explicit-maxPartitions question. I took your suggestion: the bypass stays, the
clamp stays, and an explicit split granted while enough queries are in flight that the adaptive
default would have refused now logs a throttled WARNING. The setting is JVM-wide and
long-lived, so whoever configured it is rarely the person watching latency later.

[claude]

Review: split a sparse-vector top-K into parallel RID ranges (#4085)

Read the whole diff plus the surrounding transaction/pool code. This is high-quality work: the correctness argument (ranges partition the RID space, so every doc is scored once and merge-then-keep-k is exact) is sound, the tie-break change that makes split and serial results identical is the right fix, and the adaptive load gate is genuinely well-reasoned. The test coverage is excellent - range/serial equivalence at 2..16 ranges, tie determinism, edge ranges, no-nested-split, worker-leak, in-flight suppression, deterministic deadline and caller-runs-transaction tests. The iterative fixes in patches 4-7 (reservation leak on throw, caller-runs rollback, the CI deadlock from a racing saturation count) show careful attention.

A few points, none blocking:

1. callerHoldsUncommittedChanges() and nested transactions (worth confirming)

The guard reads ctx.getLastTransaction(), which returns the innermost transaction on the stack (DatabaseContext.getLastTransaction -> transactions.getLast()), and hasChanges() is per-TransactionContext. So in a nested transaction where an outer tx has modified pages but the active innermost one has not yet, the guard returns false and the query is allowed to split - and the workers, reading committed pages through a fresh context, would miss the outer tx's uncommitted writes. The single-transaction case the comment describes (a flush inside the caller's tx) is covered because those changes land on the active tx; it's only the nested case that could slip through. Nested transactions are uncommon on this path, so this may be unreachable in practice - but since the whole point of the guard is to remove the "sees-stale-data-inside-its-own-transaction" class of bug, it's worth a sentence confirming the nested case is either impossible here or acceptable.

2. Load-gate blind spot: topKGrouped doesn't register as in-flight

topK brackets its body with pool.queryStarted() / queryFinished(), but topKGrouped does not. Grouped queries still run on caller threads and compete for CPU, yet they're invisible to tryReserveWorkers's inFlightQueries * 2 > ceiling gate. A plain topK arriving alongside heavy grouped load would still see an "idle" gate and split, taking throughput from the grouped queries - the exact failure mode the gate exists to prevent, just from a source it cannot see. If that is intentional (grouped stays serial and fans out per-bucket anyway), a one-line note would help; otherwise consider counting grouped queries too.

3. The * 2 gate factor is a hardcoded heuristic

The PR itself flags the load gate as "the piece most likely to want tuning on other hardware," and SPARSE_VECTOR_SCORING_MIN_POSTINGS_FOR_PARTITIONING got its own knob - but the inFlightQueries.get() * 2 > ceiling threshold in tryReserveWorkers is a bare literal. Given you are explicitly inviting per-hardware tuning of exactly this decision, consider pulling it into a named constant or a GlobalConfiguration entry so operators can adjust it without a rebuild.

4. Minor: every serial query now touches the pool singleton

topK calls SparseVectorScoringPool.getInstance() + queryStarted() unconditionally, including when SPARSE_VECTOR_SCORING_MAX_PARTITIONS == 1 (splitting globally off). Construction is cheap (threads are lazily created) and the atomic is negligible, so this is purely a nit - but when splitting is globally disabled the in-flight bookkeeping buys nothing, and an early return before the pool interaction would keep the disabled path free of it.

Nits

• The reservation ceiling is executor.getMaximumPoolSize(), while actual concurrency is reserved + 1 (the caller runs a range too). That is intended and good - just noting the caller thread is deliberately "extra" concurrency beyond the pool ceiling.
• Studio card rendering is acknowledged as not visually verified; the PoolMetricsTest JSON/gauge contract coverage is a good substitute, but a quick browser check before merge would close the loop.

Overall: correctness reasoning and tests are strong, the adaptive gating is thoughtfully justified and measured, and the earlier review rounds already caught the sharp edges (transaction rollback under caller-runs, reservation leaks). Points 1 and 2 are the two I would want a maintainer's confirmation on before merge.

[tae898]

#5518 reproduces on real SPLADE: 3.44x at 1M. Your 3.5x holds on Big-ANN's SPLADE-cocondenser corpus, not just a SPLADE-shaped one.

Wheel built from issue-4085-parallel-topk merged onto our main, engine jars compiled locally and passed via JAR_LIB_DIR; SparseVectorScoringPool verified present in the jar before measuring. Baseline is released 26.8.1.dev21 on the same host, same 995-query set, cpuset 0-11, INT8, settled, N=3, median.

tier	maxPartitions=1 (serial)	=0 (adaptive default)	=8 (forced)
1M	39.31 ms	11.42 ms (3.44x)	13.30 ms (2.96x)
100k	7.20 ms	4.25 ms (1.69x)	4.11 ms (1.75x)

recall@10 identical across all six cells, 0.9935 at 1M and 0.9950 at 100k, so the split is exact on our data as well as your tests.

Two things you may not have measured.

Your adaptive default beats a forced split at 1M, 11.42 against 13.30 ms. Whatever the heuristic is doing with minPostingsForPartitioning and the worker claim, it picks better than a fixed 8 on this corpus. I would not have guessed that, and it argues for leaving the default at 0 rather than documenting 8 as a recommended value.

The 100k tier gains much less, 1.69x against 3.44x. That fits your "pays in proportion to how much work the query does" exactly: our 1M queries carry 1.3M to 4.3M summed postings against a 200k threshold, so partitioning engages on essentially all of them, while at 100k a good share of queries sit near or under the gate.

What it does to the comparison. At 1M against Qdrant's 2.91 ms, we go from 12.7x to 3.9x. For context on the trajectory, the same tier measured 165 ms when we first filed the cliff, so this is 165 -> 36.8 (released dev21) -> 11.4 with your split.

How I will report it. Prose only for now, marked development line, since the branch is unmerged and our tables only carry released wheels. If it merges and ships, it moves into the table on the next release.

The caveat is yours and I will carry it. Our lane is single-stream, so we measure the favourable end of a latency-versus-throughput trade; your 1.89x total CPU at 8 ranges is real and our numbers do not show it. Anyone reading our sparse row should know the split buys latency with cores on an idle box. If you want, I can run a concurrent-client arm to put a number on what it costs under load, which is the half our harness currently cannot see.

Running now at 8.84M, serial versus adaptive, since that is where the traversal does the most work and where the gap to specialists is widest. I will post it whichever way it goes.

[lvca]

All four addressed in 1cdb06968, and two of them were real holes rather than polish.

1. Nested transactions - you were right, and it was reachable. LocalDatabase.begin() on an already-active transaction pushes a nested one, so a caller sitting in a fresh inner transaction over a dirty outer one reported clean, split, and its workers would have read committed pages without seeing what the outer transaction had written. The guard now checks every transaction on the stack. There is a test for it, and I verified it fails against the innermost-only version with "a nested-clean caller over a dirty outer transaction must not split", so it is pinned rather than asserted.

2. topKGrouped invisible to the gate - also right, and not intentional. Grouped queries never split, but they run a full traversal on the caller's thread and compete for the same cores. Unregistered, a plain topK arriving alongside heavy grouped traffic saw an idle gate, split, and took throughput from it - the failure the gate exists to prevent, arriving from a source it could not see. Now registered, on the same condition as topK.

3. The * 2 literal. Named CALLER_LOAD_GATE_FACTOR, with what it means and the 1/4/16-client measurements behind it in the Javadoc. I kept it a constant rather than a setting: it shapes only the middle of the range, and an operator who wants splitting off or always-on already has sparseVectorScoringMaxPartitions for both, so a third knob would be easy to set wrongly and hard to reason about. Named so that whoever finds a machine where the shape is wrong can find it.

4. Serial path touching the pool. Taken - with maxPartitions=1 the query now returns before getInstance(). Worth more than a nit for embedded users: getInstance() builds a ThreadPoolExecutor that the JVM has just been told it will never use.

Nits. The caller-as-extra-concurrency point is exactly as you read it. The Studio browser check is still not done and I am still not claiming otherwise.

---

Separately, @tae898 has now run this branch on the real Big-ANN SPLADE corpus, and it reproduces: 1M 39.31 ms serial to 11.42 ms adaptive, 3.44x, recall@10 identical across every cell. Two things from his run worth folding into the record here:

• The adaptive default beats a forced =8 at 1M, 11.42 against 13.30 ms. So 0 is the right default and we should not document 8 as a recommended value.
• 100k gains only 1.69x, which matches the "pays in proportion to the work" caveat: at that tier a good share of queries sit near or under the 200k posting threshold and never split.

He is carrying the CPU-cost caveat explicitly, since his lane is single-stream and therefore measures the favourable end of the trade. He has offered a concurrent-client arm to put an independent number on what it costs under load, which is the half our own measurements cover and his currently cannot.

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 high · 8 minor

Alerts:
⚠ 9 issues (≤ 0 issues of at least minor severity)

Results:
9 new issues

Category	Results
ErrorProne	1 high
CodeStyle	8 minor

View in Codacy

🟢 Metrics 12 complexity

Metric	Results
Complexity	12

View in Codacy

_{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.}

[claude]

Review: split a sparse-vector top-K into parallel RID ranges (#4085)

Reviewed the full 8-commit diff. This is a strong piece of work: the correctness argument (ranges partition the RID space, merge keeps best-k, tie-break made deterministic so the split is indistinguishable from serial) is sound and backed by ParallelRangeTopKTest, and the adaptive gating is genuinely careful about not stealing throughput under load. The commit history shows several review rounds already closed real holes (reservation leak, in-flight double-count, caller-runs transaction rollback, nested-transaction guard, self-deadlocking saturation loop). Comment/Javadoc density is excellent. A few observations below, only one of them a concrete code point.

1. The reservation-leak guard in planPartitionBoundaries doesn't catch the exception it was written for (minor / defensive)

PaginatedSparseVectorEngine.planPartitionBoundaries is declared throws IOException, and the guard added to protect the worker reservation is:

try {
  final RID[] boundaries = new RID[partitions - 1];
  ...
  return boundaries;
} catch (final RuntimeException | Error e) {
  pool.releaseWorkers(partitions - 1);
  throw e;
}

The comment states the intent explicitly: "a future edit that adds a page read here would otherwise leak the claim permanently." But a page read throws IOException, which this catch (RuntimeException | Error) does not intercept. If such an edit is ever made, the IOException propagates past this block; topK's outer finally runs pool.queryFinished() but not pool.releaseWorkers(...) (that release only lives inside the if (boundaries != null) branch), so the reservation leaks exactly as the comment warns. Harmless today because the boundary walk touches only in-memory block metadata, but since the block exists purely to guard future edits, catch (final Throwable e) (or adding IOException to the clause) would actually deliver on that promise.

2. Tie-break is a global, user-visible behaviour change

Already well-flagged in the PR description and in RidScoreMinHeap.compareEntry's Javadoc, but worth restating for release-notes visibility: the RID-ascending tie-break lives in the collector, so it applies to every query including serial ones and topKGrouped, not only split queries. Anyone whose scores tie can see a different retained set than a prior release. The new behaviour is the better one (stable across insertion order) but it is not opt-in - make sure it lands in the changelog.

3. Load-gate heuristic (already called out by the author)

CALLER_LOAD_GATE_FACTOR = 2 and the "queries in flight, not pool activity" signal are well-reasoned and the measurements are convincing. Agreeing with the author's own note that this is the piece most likely to want tuning on other hardware. Keeping it a named constant rather than a knob is the right call given sparseVectorScoringMaxPartitions already covers off/always-on.

Positives worth calling out

• MultiBucketSealedSegmentFanoutTest pins down a real latent contract (worker threads needing a DB context via checkDatabaseIsOpen's side effect) that had zero coverage - good catch independent of this feature.
• The caller-runs / bounded-queue deadlock analysis (isPoolThread() guard, "never block the submitting thread" in the saturation tests) is exactly the reasoning these pools need, and the deadline + transaction-safety tests are deterministic rather than timing-raced.
• Metrics: registering the split gauges only for sparse_vector and rendering a dash (not a zero) elsewhere is the correct disambiguation, and PoolMetricsTest pins both the registry and the JSON contract.
• CLAUDE.md pool/wire-protocol conventions respected; no new dependencies; final usage and brace style match surrounding code.

Nothing here is a blocker. Fixing the catch clause in point 1 to match its stated intent is the only change I'd actively recommend.

[lvca]

Point 1 is a fair hit and fixed in 77e9072cc. The guard caught RuntimeException | Error while its own comment said it existed for a future page read - which throws IOException and would have sailed straight past it. Worse, topK's finally cannot cover that case either, since it only releases inside the branch where boundaries came back non-null, so the claim would have leaked precisely as the comment warned. A guard that documents an intent it does not implement is worse than no guard, because the next person reads the comment and believes it.

Rather than adding IOException to the clause, I replaced it with a handed-off flag and a finally. Enumerating exception types is how this kind of guard fails quietly - it is the same mistake one level down - whereas a finally covers checked, unchecked, Error, and any future early return that forgets the claim exists.

On point 2, the tie-break in the changelog. Agreed it needs to be there and it is not something a reader should have to infer from a PR body. Flagging it for @lvca to place, since release notes are his call rather than something I should be writing into the tree. The shape of it: any query whose scores tie can return a different retained set than a prior release, including serial queries and topKGrouped, because the ordering lives in the collector rather than in the split path. The new answer is the stable one - the old set depended on heap array layout, so it was not reproducible across insertion orders - but it is a behaviour change and not opt-in.

Point 3, agreed, no change.

Thanks for the read on MultiBucketSealedSegmentFanoutTest. That one was a side effect of hitting the missing-context wall from the other direction, and you are right that the contract it pins - worker threads getting a database context only as a side effect of checkDatabaseIsOpen - is worth having covered independently of this feature.

Two things still open on my side, unchanged: ha-integration-tests is failing and I have not yet diagnosed it, and the Studio card has still not been looked at in a browser.

[claude]

Review: PR #5518 - split a sparse-vector top-K into parallel RID ranges

Thorough, well-engineered change. The commit history shows several real review holes already found and closed (reservation leak, caller-runs transaction rollback, nested-transaction guard, per-bucket load double-count, the CI deadlock in the saturation test), and the test suite is genuinely adversarial rather than happy-path. The gating rationale (reservation as a claim rather than an observation, counting queries-in-flight rather than pool activity, the getActiveCount() lock trap) is sound and matches the engine-concurrency guidance about not oversubscribing dedicated pools. Nice work.

A few points, from most to least substantive.

1. The "exactly the serial result, ties included" claim has a gap the tests don't reach (correctness / test coverage)

The PR states the split returns the serial documents and ranking, with only "scores differ by one ulp." But an ulp-level score difference and the tie-break are not independent:

• assertSameResult compares scores with Offset.offset(1e-4f) but requires the RID at each position to match exactly.
• The tie test (buildCorpus(..., allTied=true)) uses weight 0.5f on 4 dims, so every score is 2.0 computed as 0.5+0.5+0.5+0.5 - exactly representable, order-independent. That deliberately removes the ulp, so the one interaction that can actually diverge is never exercised.
• With real INT8/quantized corpora, exact ties are common, and MaxScore sums a document's terms in an order that follows the essential/non-essential split, which a range reaches at a different watermark than the serial scan. Two documents that tie exactly in the serial scan can end up an ulp apart in the split shape, and if they straddle the k/(k+1) boundary the retained set (not just order) can differ from serial.

So the strong equivalence claim is really "identical for distinct scores and for exactly-representable ties." Suggest either softening the wording in the docs/commit ("same documents up to ulp-level ties at the k boundary") or adding an equivalence case built on a quantized corpus with genuine near-ties at the boundary. The LSMSparseVectorIndexLargeBenchmark isEqualTo assertion passing on one 10M query is reassuring but not a guarantee.

2. queries.in_flight gauge no longer matches its description (docs / observability)

After the patch-4 split of counters, getInFlightQueries() returns only inFlightQueries (caller-thread queries) and excludes poolThreadQueries (per-bucket fan-out running on workers). The gauge description still reads "top-K queries executing right now, split or serial," which now under-reports when per-bucket fan-out is active. Minor, but an operator reading the Studio card during heavy multi-bucket traffic would see a lower number than reality. Worth a word in the description, or expose the pool-thread count too.

3. Minor

• callerHoldsUncommittedChanges() reaches into ctx.transactions directly and walks it by index. It is the caller's own thread-local, so there's no concurrency hazard, and the public field is consistent with how the rest of the engine touches it - fine, just flagging the coupling in case a hasUncommittedChanges()-style accessor would read cleaner.
• Studio: parallel_scan (mentioned in the updated comment) falls through to the poolKey label and renders dashes for the three split columns, which is the intended behavior - no action needed, just confirming it is covered.

Style / conventions

Consistent with the codebase: final throughout, no fully-qualified names left in main code (the one in the test was cleaned up), @Tag decision on ParallelRangeTopKTest is reasonable and justified. No System.out debug left behind (the benchmark's printf is intentional reporting). No new dependencies. Good adherence to CLAUDE.md.

Overall this looks close to mergeable; item 1 is the only one I would want addressed before merge, and it can be handled with a documentation softening plus one extra test case.

[tae898]

On review item 1, the tie/ulp gap: we may be able to supply the missing evidence, because our sparse lane runs exactly the corpus class the item says is untested, real Big-ANN SPLADE-cocondenser at INT8.

What we already have. Across serial (maxPartitions=1), the adaptive default (=0) and forced 8 ranges, on 995 dev queries at both 100k and 1M, N=3 each, recall@10 is identical to four decimal places in all six cells: 0.9950 at 100k and 0.9935 at 1M.

What that is worth, honestly: less than it looks. Recall is computed against Big-ANN's exact ground truth, so it counts how many of the true top-10 were returned, not which documents came back. Two different retained sets can score the same recall, which is precisely the failure the review describes (a tied pair straddling k/(k+1) swaps one document for another of equal score). So our numbers are consistent with set equivalence and are not evidence of it. I would not want them cited as if they closed the question.

So I have queued the direct check. One build of the 1M INT8 index, then three query passes over the same 1000 queries at maxPartitions 1 / 0 / 8, comparing the returned id lists per query and reporting two counts separately:

• queries where the returned set differs from serial (the correctness question)
• queries where the set matches but the order differs (the ulp-level tie-break, which the PR already scopes)

If the set count is zero across 1000 real quantized queries, that is a stronger statement than the current tie test can make, since our corpus has the genuine near-ties the 0.5*4 = 2.0 fixture removes by construction. If it is non-zero, you get concrete examples with the differing ids, which is the case worth softening the wording for.

It is behind an 8.84M job on our box, so a few hours out. I will post the counts whichever way they come, including if they contradict the equivalence claim.

One thing I cannot test from here: the review is right that the divergence depends on the order MaxScore sums a document's terms relative to where a range picks up the watermark. Our probe only observes the outcome, so a zero result is empirical evidence over one corpus and query set, not a proof over the algorithm.

[tae898]

Both probes I promised are done, on real Big-ANN SPLADE at 1M, INT8, released dev21 as the baseline and this branch's wheel for the split arms. One answers the tie/ulp question, one prices the split under load and speaks to the open review point about explicit maxPartitions on a busy box.

1. Equivalence on 1000 real quantized queries: no divergence at all

1000 dev queries, k=10, three settings, comparing the returned id lists against maxPartitions=1 query by query. I measured the guarantee you now state rather than the one the PR originally claimed, so: same count, rank for rank indistinguishable in score, and nothing ranking below the serial k-th.

vs serial	different SET	same set, different ORDER	different COUNT	rank-score mismatch	below serial k-th	max rank-for-rank score delta
adaptive (=0)	0	0	0	0	0	9.0e-06
forced 8	0	0	0	0	0	9.0e-06

So on this corpus the split is not merely as good as serial, it is byte-identical: same documents, same order, every time.

The interesting number is the one that is not zero. The maximum rank-for-rank score difference is 9.0e-06, on scores of order 20, so about 0.45 ppm. That is direct evidence the summation-order effect you described is real and measurable in the arithmetic. It just stays roughly four orders of magnitude below what would be needed to flip a decision at the k boundary on this weight distribution.

What this does not do is prove the corner cannot happen. It shows it did not arise across 1000 real queries at three partition settings. Your weakened guarantee is still the right one to state, and the plateau test you added is still the thing that pins it, because it constructs the case deliberately rather than hoping to meet it. If anything this pair is the useful combination: your test proves the bound holds when the case occurs, and this says the case is rare enough on real SPLADE that nobody should expect to see it.

2. Under load: your gate works, and forcing a split is worse than not splitting

Closed loop, N Python client threads against one index, 30 s per cell, same 1000 queries. CPU is charged from the container cgroup, so it counts worker threads, which a per-query wall clock cannot see.

clients	serial p50 / qps / cpu-ms-per-q	adaptive	forced 8
1	39.40 / 19.4 / 53.2	11.64 / 71.7 / 145.8	13.65 / 61.0 / 105.2
2	38.52 / 39.3 / 51.8	19.35 / 72.6 / 129.5	22.24 / 77.4 / 139.1
4	43.64 / 69.3 / 57.9	29.99 / 81.6 / 128.4	45.98 / 81.0 / 147.5
8	66.89 / 93.5 / 85.7	62.60 / 94.8 / 84.2	96.00 / 80.8 / 148.4
16	111.06 / 106.7 / 112.0	111.90 / 105.9 / 112.8	194.00 / 80.9 / 148.3

Your 1.89x corroborates. Forced 8 at one client costs 1.98x serial's CPU per query, measured end to end through the engine on a different harness and corpus. Adaptive at one client costs 2.74x, buying 3.39x on p50 and 3.70x on throughput.

The load gate demonstrably engages. Adaptive's CPU per query falls 145.8 to 112.8 as clients rise and lands on serial's 112.0 at 16 clients (and 84.2 against 85.7 at 8). It stops splitting under load, which is exactly what the gate is for, and it is visible in CPU rather than inferred from latency.

Overriding the gate costs the overrider, not only everyone else. Forced 8 keeps splitting regardless of load, its CPU per query sits flat at ~148 from 2 clients on, and by 16 clients it delivers 0.76x serial's throughput with a p50 1.75x worse than serial. On the open question of whether explicit maxPartitions > 1 should yield under load, the data says the current sharp edge cuts both ways: an operator who forces it on a busy box makes their own queries slower too, not just their neighbours'. That is an argument for yielding, or at minimum for the warning you already added being loud.

One genuine subtlety in forced 8's favour. It trades median for predictability. At 16 clients its p99/p50 is 1.53x against serial's 5.21x (297 vs 194, against 579 vs 111). Every query gets the same 8 ranges, so per-query cost is uniform and the tail compresses even as the median degrades. If someone is chasing tail latency under load rather than median, that is not a nonsensical thing to want.

The censoring, stated plainly

Serial throughput scales only 5.51x from 1 to 16 clients, so my harness has a ceiling: concurrency is driven from Python threads, and while JPype releases the GIL for the engine call, marshalling and row counting do not. Cells at 8 and 16 clients are limited by that boundary, not by ArcadeDB. Linearity holds to 2 clients (2.03x), is 89% of linear at 4, and 34% at 16.

That matters for one conclusion in particular, so I want to be explicit rather than let it pass: adaptive converging on serial at 8 and 16 clients could in principle be "both pinned at my ceiling" rather than "the gate engaged." The reason I think it is the gate is the CPU column, which is CPU divided by completed queries and therefore not ceiling-bound. If adaptive were still splitting it would still burn about 2x serial's CPU per query no matter how few queries finished. It burns 1.0x. Forced 8, which bypasses the gate, stays at 1.3x to 2.7x throughout and plateaus at 81 qps, comfortably under serial's 107, so its loss is not a ceiling artefact either.

What I cannot measure from here: maxPartitions is JVM-global, so there is no way for me to run one forced client alongside adaptive ones and watch the adaptive queries stop splitting. Every arm above sets one value for all clients. The forced-8-under-load rows price the same trade from the other side, but the true mixed-tenant case needs a per-query knob that does not exist, and nothing here should be read as covering it.

The 8.84M serial-versus-adaptive arms are running now and I will post them when they land.

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 high · 9 minor

Alerts:
⚠ 10 issues (≤ 0 issues of at least minor severity)

Results:
10 new issues

Category	Results
ErrorProne	1 high
CodeStyle	9 minor

View in Codacy

🟢 Metrics 12 complexity

Metric	Results
Complexity	12

View in Codacy

_{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.}

[tae898]

Opened #5561 for the CI redness, since it was agreed it deserved its own issue rather than a footnote here.

One correction to what we concluded above, from pulling the per-run job data: it is two separate phenomena, not one. ha-integration-tests, unit-tests and slow-unit-tests do fail in a varying subset, which matches the flakiness read. But integration-tests failed in 6 of 6 sampled runs with the same two classes every time, Issue4141SessionManagementIT and Issue4141BoltSessionManagementIT, verified identical at both ends of a 23-hour window on different commits. That one is deterministic: a plain POST /api/v1/command/graph with no session returns 400. It should reproduce locally without CI.

Nothing there touches this branch. Detail and run links are on #5561.

[claude]

Review: parallel RID-range split for sparse-vector top-K (#4085)

Reviewed the full diff (engine, server metrics, Studio). This is high-quality work: the concurrency model is careful, the reservation/load-gate design is sound, and the test suite is genuinely thorough (range/serial equivalence at 2..16 ranges, tie determinism, nested-split prevention, worker-leak, in-flight suppression, caller-runs transaction safety, live-memtable split, deadline release). I traced the correctness argument and could not find a bug. A few notes below, mostly minor.

Correctness (verified, no issues)

• Tie-break consistency. RidScoreMinHeap.compareEntry orders "score, then RID reversed" so the root is the correct eviction target, while offer keeps the primitive score > scores[0] admission. That is safe because DAAT emits candidates in ascending RID order, so a later (higher-RID) tie can never displace an incumbent. The merge then re-ranks with BY_SCORE_DESC (score desc, RID asc) and takes the first k. The global-lowest-k-RIDs set is a subset of each range's retained set, so the merge reproduces the serial result exactly for ties. Solid, and the tie test pins it.
• Boundary math. Both the adaptive and explicit paths guard byLayout < 2 up front and clamp partitions <= blocks / MIN_BLOCKS_PER_PARTITION, so block index b = blocks*i/partitions is always in-range and boundaries are distinct. No out-of-bounds.
• Claim accounting. The handedOff finally in planPartitionBoundaries plus the releaseWorkers(plan.reservedWorkers()) finally in topK make reserve/release balanced on every path, including the timeout path (confirmed by aRangeThatNeverGetsAWorkerFailsTheQueryOnTheDeadline). Carrying reservedWorkers in the plan record rather than recomputing it is the right call.
• Context handling under caller-runs. The create-only-if-absent / tear-down-only-what-I-created pattern in parallelTopK, plus the callerHoldsUncommittedChanges() guard walking the whole transaction stack, correctly avoids the "ROLLBACK PREVIOUS TXS" hazard when a range executes inline on the caller. Nicely covered by tests.

Minor observations

1. Off-by-one in the explicit-split warning. warnExplicitSplitUnderLoad logs "%d queries were already in flight" using inFlightQueries.get(), but queryStarted() has already counted the current query, so the reported figure includes the query doing the splitting (min 1 even for a lone forced query). The gate math intentionally counts self, so this is only a cosmetic wording issue in the log line - consider inFlight - 1 for the message, or reword to "in flight (including this one)".

2. Pool is now constructed for single-bucket serial queries too. With the default (maxPartitions=0), every topK calls SparseVectorScoringPool.getInstance() to register in-flight load, whereas previously an embedded single-bucket-only deployment only built the pool for multi-bucket fan-out. Threads are daemon and created lazily by the executor, so the cost is small, but it is a behavior change for embedded users who never split. maxPartitions=1 still short-circuits before getInstance(), so the escape hatch exists - just worth confirming this is intended.

3. User-visible tie-break change. The PR body already flags that tied scores now deterministically return the lowest RID for every query, split or not. Since a result set that ties on score can differ from a prior release, this is worth a line in the CHANGELOG / release notes so users aren't surprised.

4. Comment density. The Javadoc on several private methods runs to multiple paragraphs. It is genuinely informative and matches the surrounding style in this module, so not a request to change - just noting that the rationale-per-line is unusually heavy and a future reader will want the "why" kept in sync if the heuristic is tuned.

Test coverage

Excellent. ParallelRangeTopKTest covers the equivalence claim precisely (including the honest "near-tie plateau returns an equally good, not identical, result" boundary case), and MultiBucketSealedSegmentFanoutTest closes a real prior gap (the segment-backed fan-out previously depended on checkDatabaseIsOpen creating a context as a side effect, which nothing documented). The Studio gauges are pinned by PoolMetricsTest at both the Micrometer and JSON-contract layers. The one acknowledged gap - visual rendering of the Studio card - is low risk given the contract tests; the header / colspan / <td> counts line up at 9.

On the reviewer questions

• The load gate keying on queries-in-flight rather than getActiveCount() is the right instinct; the measured 14% throughput regression from pool-activity sizing justifies it well.
• Whether explicit maxPartitions > 1 should still yield under load: I'd lean toward keeping the current "explicit means do not throttle me" semantics, given the throttled WARNING already surfaces the trade. If anything changes, an explicit-but-still-yielding third mode would be additive rather than a change to the current contract.

Nice work - the notes above are non-blocking.

[tae898]

On the one open reviewer question, explicit maxPartitions > 1 under load: the two reviews above lean opposite ways, so here is the measurement, from the concurrency arm I posted earlier in this thread.

"Explicit means do not throttle me" stops being a trade and becomes a pure loss at about 4 concurrent queries. 1M INT8, closed loop, 12 cores, 30 s per cell:

clients	serial p50 / qps	forced 8 p50 / qps	forced 8 vs serial
1	39.40 / 19.4	13.65 / 61.0	2.9x better p50, 3.1x qps
2	38.52 / 39.3	22.24 / 77.4	1.7x better p50, 2.0x qps
4	43.64 / 69.3	45.98 / 81.0	p50 roughly level (0.95x), qps still 1.17x
8	66.89 / 93.5	96.00 / 80.8	p50 1.44x worse, qps 0.86x
16	111.06 / 106.7	194.00 / 80.9	p50 1.75x worse, qps 0.76x

The argument for the current semantics is that the operator knowingly bought latency with CPU. Past roughly 4 clients they are not buying anything: forced 8 is worse than serial on latency and on throughput at the same time. Its CPU per query stays pinned at ~148 ms from 2 clients on, because it never yields, while serial sits at 52 to 112. So the operator who set the knob to protect one query's latency ends up with that query slower than if they had left it alone.

That reframes the decision. It is not "let them trade throughput for latency", it is "let them make a setting that cannot help them above a handful of concurrent queries". Which argues for the gate applying to explicit values too, or for the third additive mode suggested above, rather than for keeping the current contract.

Two honesty notes. First, the crossover point is hardware and workload specific; 4 clients on 12 cores with these queries is not a universal number, and I would not put it in a doc. Second, my harness has a ceiling: serial throughput scales only 5.51x over 16x clients, so the 8- and 16-client cells are limited by our Python/JPype boundary rather than the engine. That does not rescue forced 8 though, because it plateaus at ~81 qps while serial reaches ~107, comfortably under the ceiling, so its throughput loss is real and not an artefact. The CPU column is also ceiling-independent, being CPU divided by completed queries.

What I still cannot measure is the actual multi-tenant case, one forced client alongside adaptive ones, because maxPartitions is JVM-global. If the concern is specifically an operator-forced query suppressing splitting for its neighbours, that needs a per-query knob to test properly and nothing above covers it.

[tae898]

Correction on provenance, before anyone acts on the numbers I posted an hour ago.

My wheel was built from this branch at 07:36Z today, so it predates eight commits, including 60b88fb6c and 2c861677b. I wrote that comment without having seen 60b88fb6c, and it argues for something you had already decided against two hours earlier, with better reasoning than mine. That is on me. This project has cost me one bad report already from benchmarking a wheel that did not contain the change (#5388), so I would rather flag it myself.

Your fix targets the half my harness cannot see, and it is the half that matters. The real harm in explicit splitting was never the overrider's own throughput, it was that an over-claim of up to four times the pool recorded parallelism the machine was not providing and suppressed every concurrent query for that query's whole duration. I said explicitly that I could not measure the mixed-tenant case because maxPartitions is JVM-global. You fixed exactly that case. And "a knob honoured only sometimes is worse than either extreme" is a better argument than my "the gate should apply to explicit values too", so consider that suggestion withdrawn.

What I think still stands, and why. My forced-8 arm had every client forced, so there were no adaptive queries present to be suppressed. Both before and after 60b88fb6c an explicit split proceeds regardless of capacity, so the work submitted per query is the same 8 ranges either way, and the reserved counter that changed had nobody to mislead. On that reasoning the numbers should carry over unchanged:

clients	serial p50 / qps	forced 8 p50 / qps
4	43.64 / 69.3	45.98 / 81.0
8	66.89 / 93.5	96.00 / 80.8
16	111.06 / 106.7	194.00 / 80.9

Above roughly 4 concurrent queries a forced split makes the overrider's own latency worse than not splitting, not just its neighbours'. Your rationale covers the neighbours and the honour-the-knob principle; it does not cover the operator harming themselves. I am not proposing the gate again. But if the throttled WARNING is the mechanism that surfaces this, it may be worth firing on self-harm too, not only on contention, because at 16 clients the setting is doing the opposite of what whoever set it wanted.

Flagging that as reasoning, not measurement. I have not re-measured on the current head, and the argument above is mine rather than a number. Say the word and I will rebuild and re-run the concurrency arm on 4246344b9; it is about 20 minutes of box time.

One thing that is unaffected either way. The 8.84M serial-versus-adaptive arms running now are single-stream, one query at a time, so inFlight is always 1 and none of the load-accounting commits since 07:36 can change the split decision. I will state the exact wheel commit when I post them regardless.

[lvca]

On "pure loss past about 4 clients": your p50 and qps columns support that, but your own p99 figures do not.

From the numbers you posted earlier in this thread, at 16 clients:

	p50	p99
serial	111 ms	579 ms
forced 8	194 ms	297 ms

Forced 8's tail is roughly half serial's at that load. So it is not dominated - it is a median-versus-tail trade that inverts as concurrency rises. Every query gets the same 8 ranges, so per-query cost stays uniform and the tail compresses even while the median degrades, which is exactly what someone chasing p99 under load is buying. You had already spotted this as the "one genuinely good point in forced 8's favour"; I think it survives the later framing.

That is the reading I have gone with, so the explicit setting keeps its semantics rather than yielding. What changed is what an operator is told: the warning and the setting now describe it as a median win while the box is quiet, a median and throughput loss once it is busy, and a tail win throughout - so "yes for tail-sensitive traffic, no for anything else". Quoted as absolute p99 rather than p99/p50, because the ratio reads as a worse tail when the tail is in fact lower. No crossover figure, per your point that 4-on-12-cores is not universal.

The CPU-per-query column is the most useful thing in that run, by the way. It is the first evidence that the gate actually engages rather than the latency merely looking as though it does.

[tae898]

@lvca one ask, because 00b27f575 puts my numbers in front of operators and they were measured on a wheel that predates 60b88fb6c.

Would you re-run the forced-8-under-load arm on your own harness before that wording ships? Not because I think the numbers are wrong, but because for this particular claim your harness is the better instrument and mine is measurably censored.

Why mine is the weaker one here. I drive concurrency from Python threads. JPype releases the GIL for the engine call, but argument marshalling and row counting do not, so serial throughput in my harness scales only 5.51x across a 16x increase in clients. My 8- and 16-client cells are limited by that boundary, not by ArcadeDB. Yours is JVM-native with no such ceiling, and it already produced the 1.89x CPU figure and the 14% pool-activity regression, so it can see the region where mine goes blind.

What rescued my conclusion was the CPU column, which is CPU over completed queries and therefore not ceiling-bound, plus the fact that forced 8 plateaus at ~81 qps while serial reaches ~107, comfortably under the ceiling. That is enough to say the inversion is real. It is not enough to trust the magnitudes now in GlobalConfiguration, and 0.76x and 1.75x are magnitudes.

The specific thing to check, if you do: whether the inversion survives 60b88fb6c. My reasoning says it should, because every client in my arm was forced, so no adaptive query was present for the old over-claim to suppress, and an explicit split proceeds regardless of capacity both before and after. Same 8 ranges submitted per query either way. But that is my reasoning about your code, which is exactly the kind of claim that should be measured rather than argued, and you can rebuild the branch in the time it takes me to fail at it twice (the recipe in our own build.sh header turns out to need a Maven image with tar in it).

If the magnitudes move, please take yours over mine and change the doc. I would rather the setting carry a number from the harness that can actually see 16 clients than one from mine with a footnote.

What our side is genuinely better at, and is doing instead: the 8.84M serial-versus-adaptive arms are running now on real Big-ANN SPLADE with exact ground truth, single stream, which is where our corpus and scale are the asset and where the GIL ceiling cannot bite because there is only ever one query in flight. Those I will stand behind, with the wheel commit stated.

Also worth folding into the same doc pass if you agree with it: the tail behaviour. p99 297 against 579 at 16 clients means forced splitting is a tail optimisation that costs median and throughput, which is a coherent thing for someone to want and is not obvious from the setting's name.

[claude]

Review: feat(#4085): split a sparse-vector top-K into parallel RID ranges

Reviewed the full diff (engine core, pool, heap, metrics, Studio, tests). This is a high-quality change: the correctness argument is sound, the concurrency handling is careful, the documentation is exceptional, and the test coverage is genuinely comprehensive. Notes below are mostly confirmations plus a few things worth a second look. Nothing here is blocking.

(Static review only: mvn was unavailable in my environment, so I did not recompile or re-run the suites. The PR reports the full engine (10,128) and server (763) suites green.)

What is solid

• Correctness of the split. Ranges partition the RID space disjointly ([null,b0), [b_i-1,b_i), [b_last,null)), every document is scored exactly once against the same query, and mergeRanges keeps the best k of the union. The endExclusive bound in BmwScorer.scan relies on candidates being produced in ascending RID order, which the DAAT heap already guarantees. This holds regardless of which segment supplied the boundaries.
• Tie determinism. RidScoreMinHeap.compareEntry reverses the RID leg so the root is the first to evict, and BY_SCORE_DESC ranks the merge the same way. The reasoning that admission can stay a > test (ascending arrival means a later candidate always loses a tie) is correct.
• Worker DatabaseContext handling. The create-only-if-absent / remove-only-what-was-created pattern, and specifically not calling init() unconditionally because caller-runs can execute a range inline on the user's own transaction thread, is the subtle bit and it is handled right. The callerHoldsUncommittedChanges guard walking the whole transaction stack (not just the innermost) is the correct call.
• Reservation over observation. Gating on a claimed counter rather than getActiveCount() (which is both misleading for caller-thread work and lock-contended) is a good insight, and the measurements back it.
• Resource safety. The handedOff finally in planPartitionBoundaries, the release in topK's finally keyed to plan.reservedWorkers(), and queryStarted/queryFinished symmetry (a thread's pool membership never changes mid-query) all line up. No obvious leak path.
• Tests + metrics. ParallelRangeTopKTest covers the cases that matter (range/serial equivalence, tie determinism, empty/oob ranges, no nested split, in-flight suppression, worker hand-back, uncommitted-changes on both inner and outer tx, live memtable). Benchmark is @Tag("benchmark"). Studio colspan/<th>/td counts all agree at 9, and PoolMetricsTest pins the "present on sparse_vector, absent elsewhere" contract.

Worth a second look

1. Explicit reserveWorkers ignores poolThreadQueries while adaptive tryReserveWorkers subtracts it. For the explicit path, reserved = reserveWorkers(partitions-1) saturates only at ceiling - current, not ceiling - current - onPool. Since every other query's gate reads free = ceiling - reservedWorkers - onPool, an explicit split issued while a per-bucket fan-out occupies workers can push reservedWorkers high enough to drive free negative for concurrent adaptive queries, suppressing their splitting harder than the real contention. This looks intentional ("do not throttle me"), and it is bounded/temporary, but it is a slightly stronger side effect on neighbours than the docs emphasize. Worth a one-line confirmation that this is desired. (Relatedly, question Bump slf4j-simple.version from 1.7.31 to 1.7.32 #3 in your description - whether explicit >1 should yield under load - lives here.)

2. Load imbalance on multi-segment indexes. Boundaries come from one dim of one segment. When that segment's RID span does not cover the global space (e.g. a freshly loaded second segment with higher RIDs), the final range [b_last, null) absorbs every RID past the widest segment, so one worker can carry a large majority of the corpus. You document this ("ranges can come out uneven") and correctness is unaffected, but on a freshly-loaded-before-compaction index the speedup can collapse to roughly serial while still paying the split's CPU overhead. Fine to leave, but it may be worth a note that the split can be near-useless (not just "uneven") in that transient state.

3. Tie-break is a user-visible behaviour change for all queries, not just split ones. You call this out clearly, and the new answer is the better one. Just make sure it lands in the release notes / changelog, since a result set that ties on score can differ from a previous release even with splitting disabled.

4. Explicit oversubscription + bounded queue + caller-runs. With maxPartitions up to 4 * poolSize, parallelTopK submits partitions-1 tasks regardless of reserved; once the queue fills, caller-runs executes ranges inline and serializes them on the submitting thread. This is documented and the queries.split gauge correctly counts the decision not the outcome (pairing with caller_run_fallbacks), so no correctness issue - just flagging that a large forced value can quietly degrade toward serial rather than scaling.

Minor

• The Javadoc and GlobalConfiguration descriptions are outstanding, though very long; that is a house-style choice consistent with the surrounding sparse-vector code, so no change requested.
• SPARSE_VECTOR_SCORING_MAX_PARTITIONS is read twice per query (in topK and again in planPartitionBoundaries); a mid-query flip to 1 just falls back to serial, which is benign.

Nice work - the gating rationale and the "count queries in flight, not pool activity" insight are the heart of this and they are convincing.

[tae898]

Thanks for running it. My tail claim was wrong and yours should replace it entirely.

The interesting part is not that my magnitudes were soft, it is which number my harness broke, because it is the one I should have distrusted first and did not.

Your serial p99 at 16 clients is 62.8 ms. Mine read 579 ms for the same cell. That is a factor of nine, and it is all mine: 16 Python client threads on a 12-core cpuset, with marshalling serialised behind the GIL, so the serial arm spends most of its tail waiting on my harness rather than on the engine. Forced 8's tail was inflated too, just less catastrophically, and the ratio between two differently-inflated numbers came out backwards. That is how I got a tail win out of what your harness shows is a 2.5x tail loss.

I argued the CPU column rescued my conclusions because CPU over completed queries is not ceiling-bound. That was true, and I then over-extended it: it licences the inversion, which is a statement about total work, and it says nothing about a percentile. The tail is the metric most sensitive to queueing in front of the engine, so it is exactly the one a saturating harness cannot report. I had the evidence that my harness was saturating (5.51x over 16x clients, in the same comment) and still quoted a p99 comparison from inside the saturated region. That is the lesson worth me carrying, and it generalises past this PR: our numbers are trustworthy at concurrency 1, where nothing queues, and our percentiles should not be quoted above it.

So: no measure on which forcing wins above light concurrency, and the setting is for one-query-at-a-time work with nothing else running. Your wording, your numbers.

The adaptive figures you recorded are the better headline anyway, and I would not have obtained them: at 8 clients beating serial on throughput (275 vs 196), median (35.9 vs 37.5) and p99 (75.2 vs 95.9), then at 16 clients matching serial while splitting 2 queries out of 11,444. That last figure is the gate working, stated directly. I inferred the same thing indirectly from CPU per query converging on serial's, which was the right inference from the wrong instrument. 2 out of 11,444 is what the claim should rest on.

Also good catch on the warning threshold. Firing only when the adaptive gate would have refused means ~10 queries in flight on an 18-thread pool, while the harm starts between 4 and 8, so the operator most likely to be hurt was the one least likely to be told. Keying the second trigger on forced ranges oversubscribing the pool rather than on a client count is the right shape, since it is the thing that is actually true on both our boxes despite the different core counts.

Nothing here changes the 8.84M serial-versus-adaptive arms still running on our side. Those are single stream, one query in flight, so the ceiling that broke my tail numbers cannot reach them, and real Big-ANN SPLADE at 8.8M documents with exact ground truth is the part our harness is actually the right instrument for. I will post them with the wheel commit named.

[claude]

Review: split a sparse-vector top-K into parallel RID ranges (#4085)

Reviewed the full diff. This is a high-quality change: the correctness argument for the range split is sound (disjoint RID partitions -> each doc scored exactly once -> merge-and-keep-best-k reproduces the serial result), the load gate is a genuinely thoughtful answer to the "pool looks idle because queries run on caller threads" problem, and the test suite is unusually complete (range/serial equivalence at 2..16 ranges, tie determinism, empty/out-of-bounds ranges, no-nested-split, worker-leak, in-flight suppression, caller-runs transaction safety, nested-tx guard, live-memtable equivalence, deadline release). The reserve/release accounting is symmetric by construction (PartitionPlan carries the number the pool actually recorded) and is pinned by claimedWorkersAreAlwaysHandedBack and the deadline test. Nice work.

A few observations, mostly questions rather than defects:

1. Default config now eagerly builds the scoring pool for every sparse-vector query

topK/topKGrouped only skip SparseVectorScoringPool.getInstance() when MAX_PARTITIONS == 1. Under the default (0), any sparse-vector query - including a tiny single-bucket one that will never clear MIN_POSTINGS_FOR_PARTITIONING, and every grouped query, which never splits - now constructs the JVM-wide ThreadPoolExecutor on first use. Previously the pool was built only when a multi-bucket fan-out actually occurred. This is inherent to the load-gate design (every query must register to be counted), so it may be the intended trade, but it is a resource-footprint change for embedded single-bucket deployments whose only opt-out is setting the knob to 1. Worth confirming that is acceptable, and perhaps calling it out in the MAX_PARTITIONS doc.

2. Benchmark numbers baked into user-facing setting descriptions

MAX_PARTITIONS and the pool Javadoc embed specific measured figures ("18-worker box", "500k documents", "0.52x the throughput", "1.9x the CPU"). They are great in a design doc or the PR body, but in a GlobalConfiguration description they will silently go stale as hardware/workload assumptions change, and they make an already very long help string harder to skim for the actual semantics (0/1/>1). Consider trimming the setting text to the contract and leaving the measurements in the code comments / this PR.

3. Partition balance keys off a single segment's block layout

planPartitionBoundaries derives cut points from widest.blockCount() of the single widest (segment, dim), then applies those RID boundaries across all segments + memtable. You document this well and correctness does not depend on it, but on a multi-segment index (pre-compaction) the ranges can come out lopsided and the speedup degrades to "one range finishing long after its siblings." Since partitionedQueryCount only counts the decision, there is currently no signal for skew. Not blocking - just noting the Studio card cannot distinguish "split but unbalanced" from "split and even."

4. Pure-memtable indexes never split

segSnapshot.length == 0 returns null, and totalPostings counts only segment postings, so an index with a large unflushed memtable both fails the min-postings gate and has no boundary source. Reasonable (unflushed = typically small), and the live-memtable test covers the mixed case, but a heavy write-then-query-before-flush workload silently stays serial. Fine as a documented limitation if that is the intent.

Minor

• MAX_PARTITIONS.getValueAsInteger() is read several times per query across topK, planPartitionBoundaries, and warnExplicitSplitUnderLoad. It is only a volatile read so the cost is negligible, but reading it once per query and threading the value would match the "re-read on every query" comments more literally and avoid any chance of the value changing mid-decision.
• The class/method Javadoc is very dense. It is genuinely informative here, but a couple of the longest blocks (e.g. the warnExplicitSplitUnderLoad rationale) restate the PR body nearly verbatim; a future maintainer might appreciate the "why" being one layer terser.

Things I checked and found correct

• Reserve/release symmetry on every path incl. explicit-clamped-at-ceiling and the handedOff finally guard against a future page-read leak.
• Nested-split prevention via POOL_THREAD set once in the thread factory (survives per-task), and the per-bucket fan-out correctly gated by isPoolThread().
• Caller-runs safety: create-context-only-if-absent + tear-down-only-what-was-created avoids the DatabaseContext.init "ROLLBACK PREVIOUS TXS" branch clobbering the caller's transaction; well covered by aRangeForcedOntoTheCallerThreadLeavesTheCallersTransactionAlone.
• callerHoldsUncommittedChanges walks the whole transaction stack, not just the innermost (nested-clean-over-dirty-outer test confirms).
• Tie-break total order (compareRid) is consistent across the per-range heap, the merge comparator, and the range end bound; the -1 no-end sentinel is safe since bucket ids are non-negative.
• Studio empty-state colspan (9) matches the 9 headers.

The behaviour changes are both correctly flagged in the PR: (a) tie-break on RID-ascending is now applied to every query, split or not, so a result set that ties on score can differ from a prior release, and (b) the one-ulp summation-order difference at the k boundary, which the plateau test bounds rather than eliminates. Both are documented on the tests that assert them, which is the right call.

Overall: LGTM pending a maintainer's read on point 1 (eager pool construction under the default) and the heuristic-tuning caveats the author already highlighted.

Automated review by Claude. Please verify the observations above against your own judgement.

[lvca]

Retracting my p99 argument above: I re-ran it here as you asked, and your original "pure loss" conclusion was right. Mine was wrong.

1M INT8, 18-thread pool, JVM-native clients, 30 s cells, on bad4eaf28:

clients	serial p50 / p99 / qps	forced 8 p50 / p99 / qps	adaptive p50 / p99 / qps
1	32.7 / 35.7 / 32	8.8 / 12.8 / 112	8.6 / 13.5 / 110
4	37.4 / 51.1 / 105	21.1 / 45.9 / 177	12.9 / 70.5 / 175
8	37.5 / 95.9 / 196	50.0 / 115.9 / 149	35.9 / 75.2 / 275
16	39.5 / 62.8 / 391	73.2 / 156.4 / 204	41.1 / 56.2 / 382

The tail benefit does not reproduce. At 16 clients forced 8's p99 is 156.4 ms against serial's 62.8, two and a half times worse. Your 297-against-579 was the one thing making forced splitting a coherent choice under load, and it does not survive a harness without the Python boundary. So there is no regime above light concurrency where the setting wins - not median, not throughput, not tail - and my "median-versus-tail trade" reading was wrong.

You were also right that the magnitudes needed our instrument, and they moved against the setting, not for it: 0.52x throughput rather than 0.76x, and a median 1.85x worse rather than 1.75x. Your 1.98x CPU per query corroborates the 1.89x measured here a different way, so the two harnesses agree on the cost and disagree only where yours is censored.

Both the WARNING and the setting's documentation now carry these numbers instead of yours, and say plainly that forcing is worth it only when one query at a time must be as fast as possible with nothing else running. The warning also fires earlier: its only trigger was "the adaptive gate would have refused this", which needs half the pool's worth of queries in flight, while the crossover you identified sits well below that. A second trigger keyed on the forced ranges oversubscribing the pool covers the region where the setting has already turned against whoever set it - which was your point, and it was a good one.

On the semantics themselves we are leaving explicit maxPartitions > 1 as it is, honouring the knob rather than gating it. That now rests on one argument alone, that a setting honoured only sometimes is harder to reason about than one that always does what it says, and no longer on any performance case. The measurements are in the log line and the docs so nobody sets it by accident.

One thing worth having from your run that ours does not show: the adaptive default at 8 clients beat serial on all three of throughput (275 vs 196), median (35.9 vs 37.5) and p99 (75.2 vs 95.9), and at 16 clients matched serial while splitting 2 queries out of 11,444. The gate is not merely getting out of the way under load, it is finding a middle band where splitting still pays. That band is what your single-stream 8.84M arms cannot see and ours cannot corroborate at scale, so the two runs are complementary rather than redundant.

Thank you for flagging the wheel provenance and your own ceiling unprompted. Both times it was the thing that made the numbers usable.

[tae898]

Our retractions crossed, so we agree on the tail: I withdrew that claim a few minutes before your comment landed, for the same reason you found it. Good outcome either way, and no need to relitigate it.

But there is something in your table neither of us has remarked on, and it is about the default rather than the override.

Adaptive's p99 against serial's, from your own numbers:

clients	serial p99	adaptive p99	adaptive vs serial
1	35.7	13.5	0.38x, much better
4	51.1	70.5	1.38x worse
8	95.9	75.2	0.78x better
16	62.8	56.2	0.89x better

Adaptive's tail is better than serial everywhere except at 4 clients, where it is worse than serial and worse than forced 8 (45.9). That is non-monotonic, and it sits exactly in the middle band you just identified as where the gate is doing something clever rather than merely getting out of the way. At that point its median is excellent, 12.9 against serial's 37.4, so it is buying a 2.9x median win with a 1.38x tail loss. That is a real median-versus-tail trade. It just belongs to the default, not to the setting you were documenting.

A mechanism that would explain it, offered as a hypothesis rather than a finding. Four clients is presumably where the gate starts refusing some queries and not others. A refused query then runs unsplit while pool threads are still busy with other queries' ranges, so it is slower than the same query would be on an all-serial box, which is why adaptive's p99 exceeds serial's rather than merely matching it. Split and unsplit queries coexisting gives a bimodal distribution, and p99 reads the upper mode. Forced 8 has no such mixture (everything splits) and serial has none either (nothing does), which fits both of them having tidier tails at that point.

Cheap ways to tell whether that is what it is, if you think it is worth knowing: the split rate at 4 clients (you already instrument it, you quoted 2 of 11,444 at 16), and whether the 4-client latency histogram is bimodal rather than long-tailed. If the split rate at 4 is somewhere in the middle rather than near 0 or near 100%, that is the mixture.

Why I think it matters more than the forced-split question you have been documenting. Explicit maxPartitions is opt-in and now carries a warning; almost nobody will set it. The adaptive default is what every user gets. If the default has a tail cost in a specific concurrency band, that is worth a line somewhere even if the answer is "inherent to any gate, accepted, the median win is worth it". It may also be noise from a single 30 s cell, which is exactly why I am flagging it rather than asserting it.

I cannot check any of this myself: it lives in the region where my harness is censored, which is the whole reason you re-ran it.

On leaving explicit maxPartitions > 1 ungated, resting on predictability alone and explicitly not on a performance case: agreed, and narrowing your own justification when the performance half evaporated is the right way to have done it.

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 high · 9 minor

Alerts:
⚠ 10 issues (≤ 0 issues of at least minor severity)

Results:
10 new issues

Category	Results
ErrorProne	1 high
CodeStyle	9 minor

View in Codacy

🟢 Metrics 12 complexity

Metric	Results
Complexity	12

View in Codacy

🟢 Coverage 86.86% diff coverage · -6.39% coverage variation

Metric	Results
Coverage variation	✅ -6.39% coverage variation
Diff coverage	✅ 86.86% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (c906c6b)	146708	108811	74.17%
Head commit (dcf2aee)	179225 (+32517)	121482 (+12671)	67.78% (-6.39%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#5518)	236	205	86.86%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{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.}

[lvca]

Ran it. Your anomaly is real and your mechanism is right. Documented in dcf2aee0b.

Three reps, 1M INT8, 18-thread pool, 30 s cells:

4 clients	p50	p99	split rate
serial	26.2 / 25.6 / 26.8	30.8 / 27.3 / 31.7	0%
adaptive	7.6 / 7.6 / 7.8	32.3 / 37.7 / 48.6	73-76%
forced 8	13.7 / 14.0 / 14.7	22.0 / 21.4 / 29.8	100%

Adaptive's p99 exceeds serial's in all three reps, and forced 8 - uniform width - has the tidiest tail of the three. So it is not a single-cell artefact.

The mixture is the cause, with one refinement. It tracks the spread in how queries are treated rather than the split rate as such: at 2 clients every query splits and the tail is fine, at 4 some claim a wide split while others get none and the tail is worst, at 8 the grants are uniformly small and it is fine again. Maximum spread in the middle, which is why the band closes on both sides. Your bimodal-distribution reading is exactly what the numbers show; the variable is partition width, not split-versus-serial.

I should also withdraw a counter-argument I nearly posted. My first pass had adaptive's p99 at 2 clients worse than serial with a 100% split rate, which I read as refuting the mixture. That cell was measured on a loaded box. On a quiet one adaptive's tail at 2 clients is better than serial's, so the refutation was noise and your hypothesis never had a problem.

Documented rather than fixed. The trade at 4 clients is a 3.4x median win for about 1.2x the p99. Flattening it means giving up most of that median win to shave a tail still around 38 ms, which is the wrong way round - but you are right that a user should not have to discover it, so the setting now says the default can cost tail latency in a narrow concurrency band and quotes these numbers.

And your ask exposed a gap in what I instrumented. queries.split records whether a query split, not into how many ranges - so the metric cannot see the variable that explains this. The gauge that would is a distribution of granted partition counts. That is now written next to the metric so whoever chases it does not have to rediscover why the existing one is insufficient.

Worth noting what this run also showed about the middle band, since it is the case your single-stream arms cannot reach: at 8 clients the adaptive default beat serial on all three of throughput (275 vs 196), median (35.9 vs 37.5) and p99 (75.2 vs 95.9). The gate is not only getting out of the way under load, it finds a region where splitting still pays on every measure.

[claude]

Review: split a sparse-vector top-K into parallel RID ranges (issue 4085)

I read the full diff and traced the correctness and concurrency claims against the source (PaginatedSegmentReader, GlobalConfiguration, SparseVectorScoringPool, the Studio table). This is a high-quality change: the fan-out is correctly gated, shared state is handled carefully, and the test suite is unusually complete. Comments below, roughly in order of importance.

Correctness / concurrency (looks sound)

• Range partitioning is exact. Ranges partition the RID space, each document is scored once, and mergeRanges keeps the best k of the union. The one legitimate divergence (float summation order moving a score by an ulp at the k boundary) is documented and pinned by aPlateauOfNearTiesAtTheBoundaryStillReturnsAnEquallyGoodResult.
• Thread-safety of the read path checks out. PaginatedSegmentReader.dimMetadata caches behind an AtomicReferenceArray CAS, each range opens its own DimCursor stack, and pages resolve through the shared cache. The linchpin claim in rangeTopK Javadoc holds.
• Worker context handling is careful and correct. The create-only-if-absent / tear-down-only-if-created pair, the isPoolThread() gate against nested fan-out on a bounded queue, and the caller-runs inline case are all handled. The callerHoldsUncommittedChanges() walk over the whole transaction stack (not just the innermost) is the right call and is tested.
• Worker-reservation accounting is released in a finally on every abrupt exit, and the leak/deadline tests confirm it returns to baseline.

Worth a second look

1. The tie-break change is user-visible for every query, not just split ones. RidScoreMinHeap / BY_SCORE_DESC now break ties on RID ascending unconditionally, so a database with splitting disabled still gets a different retained set than a previous release when scores tie. You flagged this, and the new behaviour is strictly better (stable across insertion order), but it deserves an explicit line in the release notes / CHANGELOG since it is not opt-in.

2. The caller-scores-range-0-concurrently optimization is defeated under caller-runs saturation. In parallelTopK the worker ranges [1, partitions) are submitted first, then the caller scores range 0. When the queue is full, submit() runs the rejected range inline during the submit loop, so under saturation all ranges execute serially on the caller thread before range 0 even starts. This is correct and only a lost optimization (and the load gate usually prevents splitting under load anyway), but the Javadoc claim that the caller scores its own range concurrently is optimistic in exactly the saturated case that reaches this path. A one-line caveat would keep the comment honest.

Maintainability nits (non-blocking)

3. Hardware-specific numbers baked into config docs and the WARNING log. The MAX_PARTITIONS description and warnExplicitSplitUnderLoad message quote concrete figures (18-worker box, 0.52x throughput, p99 2.5x worse). Great in the PR body and design comments, but in a shipped config description and a runtime WARNING they will age into something that reads as a claim about the operator hardware rather than the machine you measured. Consider trimming to the mechanism plus the crossover caveat you already give.

4. MAX_PARTITIONS is read twice per query (the ==1 early return in topK, then again in planPartitionBoundaries). getValueAsInteger() is cheap (numeric default, no parsing), so this is fine on the hot path; just noting the double read is benign only because a mid-query setValue race is harmless here.

Test coverage

Excellent. Range/serial equivalence at 2..16, tie determinism, near-tie plateau, empty/out-of-bounds ranges, engine-level split accounting, no-nested-split, worker-leak, in-flight suppression, fully-claimed-pool fallback, deadline release, caller-runs transaction safety, nested-transaction gating, and live-memtable equivalence. The new MultiBucketSealedSegmentFanoutTest closing a pre-existing untested contract (segment-backed multi-bucket fan-out relying on checkDatabaseIsOpen side-effect context) is a good catch on its own. Studio card rendering is the one unverified piece, as you noted; the column count (9 headers, colspan=9, gaugeOrDash for absent keys) is internally consistent with PoolMetricsTest.

Overall: solid, well-reasoned, and thoroughly tested. My only ask before merge is a CHANGELOG/release note for the tie-break behaviour change (item 1); the rest are optional.

[tae898]

8.84M, as promised. It holds, and it is larger than at 1M: 3.58x.

Full Big-ANN SPLADE corpus (8,841,823 docs), INT8, 1000 dev queries, cpuset 0-11, 16g heap, settled, single stream, N=3, median.

	maxPartitions=1 (serial)	=0 (adaptive)	gain
p50	301.52 ms	84.14 ms	3.58x
p95	994.36	238.07	4.18x
p99	1250.05	304.02	4.11x
QPS	2.60	9.70	3.73x
recall@10	0.9914	0.9914	identical
build	936 s	945 s	unchanged

Per-rep p50 was [301.52, 299.69, 304.23] serial and [84.30, 82.65, 84.14] adaptive, so the spread is under 2% on both arms.

Two things worth drawing out.

The gain grows with the work, within a tier as well as across tiers. 3.58x at the median, 4.18x at p95, 4.11x at p99. The expensive queries benefit most, which is the same "pays in proportion to how much work the query does" property you described, showing up inside a single query mix rather than between 100k and 1M. It is also the useful direction: the queries that were painful are the ones that improve most, so the distribution tightens rather than just shifting.

It is bigger here than at 1M (3.58x against 3.44x), which closes the scaling question. At 8.84M essentially every query carries enough posting mass to clear the 200k gate and partition fully, whereas at 100k a good share sat under it and only got 1.69x. The three tiers now read 1.69x / 3.44x / 3.58x, monotonic in corpus size.

What it does to the comparison at the tier where we were furthest behind. Against Qdrant's 16.4 ms at 8.84M we go from 18.4x to 5.1x. For the whole trajectory on this tier: 387 ms when we were on the pre-dev21 line, 293 ms on released dev21 after your three merged rounds, 84 ms with the split. Elasticsearch answers this tier in 10.8 ms but at 0.781 recall against our 0.9914, so that one is not a like-for-like comparison and I would not quote it as one.

Provenance, since it caught me out once already today. This wheel is from 013468d80, the branch head at 07:36Z, so it predates the eight commits that landed afterwards including 60b88fb6c and bad4eaf28. For this measurement that does not bite: it is single stream, one query in flight at all times, so the load gate never engages, no reservation is ever contended, and none of the load-accounting changes since can alter the split decision. Every arm above is inFlight=1. If you want it re-run on the merged code I am happy to, but I would expect the same numbers for that reason rather than because I checked.

Congratulations on the merge. Once this ships in a release we will re-measure on the released wheel and it moves from our prose into the paper's table, which is where I have wanted to put it since the 165 ms days.