Size tables for query planning with the O(1) RocksDB key estimate instead of a full key-space scan (backport #2163 to v5.2) - #2478
Merged
Conversation
estimatedEntryCount() called RocksDatabase.getKeysCount(), a synchronous
native iteration of the entire key space, once per store per 10-second
memo window. On a 28.6M-row table that is ~11s on the main thread, and
the memo never took effect there: `now` is sampled before the call, so
`now + 10000` is already in the past when it returns. The query planner
calls it once per additional condition through intersectionEstimate and
the range heuristics, so an N-condition query paid N-1 full scans --
a 10-condition operation measured ~100s of main-thread stall with the
Operations API unresponsive throughout.
Read `rocksdb.estimate-num-keys` instead, which is O(1) and returned the
identical count on the affected table. estimate-num-keys skews high on
overwrite/delete-heavy data until compaction; every consumer is a
relative-ordering or explicitly-estimated path, so the accuracy-per-cost
trade is deliberate.
It also reports 0 for a *populated* table once accumulated tombstones
reach its non-deletions, which getKeysCount() never did -- there a 0 meant
a genuinely empty store. Zero is still the truthful count, and Table.ts
reports it verbatim as an estimated pagination total, so it is left alone
and the two arithmetic sites that cannot take it are guarded instead:
- the AND-group branch of estimateConditionForTable divides by it,
yielding Infinity;
- the `ne null` branch subtracts the index's null count from it,
yielding a negative estimate. The exact count could not underrun
getValuesCount(null); a 0 estimate can.
The divisor matters to planning: the adaptive filter/index switch derives
thresholdRemainingMisses as `estimated_count >> 4`, and Infinity >> 4 is 0
rather than NaN, so the isNaN check passes it and the first non-matching
record flips the request onto searchByIndex plus a full Set build. Guarded,
the same group estimates 200 and the threshold is a usable 12. It uses the
`|| 1` idiom already applied to the sibling relationship divisor two
branches below.
The subtraction does not affect planning -- -32 and 0 both trip the switch
on the first miss -- but a negative cardinality is not a value to hand to a
caller: on main Table.ts reports it verbatim as an estimated pagination
total. It is clamped with Math.max at 0, not 1, since `|| 1` does not catch
a negative and a table whose rows are all null has a true `ne null` count of
zero.
Backport of the estimatedEntryCount portion of #2163, whose helper and
intersectionEstimate are taken byte-identical; the two arithmetic guards
are the addition, and go to main separately.
getEstimatedKeyCount() has been present since rocksdb-js 2.4.x, so this
needs no dependency bump; the statistical range-estimate portion of #2163,
which does require 2.8.0, is deliberately not included.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request optimizes database key count estimation for RocksDB by using the O(1) getEstimatedKeyCount() method instead of iterating the entire store with getKeysCount(). It also prevents division-by-zero and negative estimates when the estimated entry count is zero. The reviewer suggested using explicit nullish and NaN checks instead of relying on simple truthiness checks to make the division in intersectionEstimate more robust.
Contributor
|
Reviewed; no blockers found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
estimatedEntryCount()sized every table for query planning by callingRocksDatabase.getKeysCount()— a synchronous native iteration of the entire key space. On a 28.6M-row analytics table that is ~11s of blocked main thread per call, and its 10-second memo never took effect there:nowis sampled before the call, sonow + 10000is already in the past by the time it returns. The planner reaches it once per additional condition, so an N-condition query paid N−1 full scans — a 10-condition operation measured ~100s of main-thread stall with the Operations API unresponsive throughout, while the data plane kept answering in 1–3ms.It now reads the
rocksdb.estimate-num-keysproperty, which is O(1) and returned the identical count (28,668,898) on the affected table.This is the
estimatedEntryCountportion of #2163, which merged tomainon 2026-08-31; the helper andintersectionEstimateare taken byte-identical from it. No dependency change is needed — v5.2 already pins rocksdb-js 2.8.0, andgetEstimatedKeyCount()has in fact been present since 2.4.x. The statistical range-estimate portion of #2163, which does require rocksdb-js 2.8.0, is deliberately excluded.The two guards, which are the addition beyond #2163
estimate-num-keysreports 0 for a populated table once its accumulated tombstones reach its non-deletions. The exact count it replaces could not do that — a 0 there meant a genuinely empty store. Measured on rocksdb-js 2.4.1: a store holding 200 live records reports an estimate of 0 after 5000 tombstones for never-written keys, whilegetKeysCount()still returns 200.Zero is still the truthful count, so the count is left alone and the two arithmetic sites that cannot take it are guarded instead:
Infinity|| 1, the idiom already on the sibling relationship divisor two branches belowne nullbranch subtracts the index null count from it-500)Math.max(…, 0)The divisor is the one that changes planning. The adaptive filter/index switch derives
thresholdRemainingMissesasestimated_count >> 4, andInfinity >> 4is0rather thanNaN, so theisNaNcheck passes it through and the first non-matching record flips the request ontosearchByIndexplus a fullSetbuild for the next 10 seconds — the inverse of the plan this change exists to protect. Guarded, the same group estimates 200 and the threshold is a usable 12.The subtraction does not change planning:
-32and0both trip the switch on the first miss. It is guarded because a negative cardinality is not a value to hand to a caller — onmain,Table.tsreportsestimated_countverbatim as an estimated pagination total, so an empty page could answerContent-Range: */-500. The clamp is at 0, not 1: a table whose rows are all null has a truene nullcount of zero, and flooring at 1 would trade a negative total for a phantom row. The same two guards go tomainseparately, since #2163 left them there.For the record, my first justification for this second guard was that it protected the adaptive switch. That was wrong — review caught it — and the guard is kept on the narrower, real ground above.
For the human reviewer
estimate-num-keyscounts entries across memtable and SSTs without reconciling overwrites, so it reads high on rewrite-heavy stores and collapses toward 0 once tombstones reach non-deletions. This repo already measured it:DESIGN.mdrecords 37,775 against an exact 4,001 on a real HNSW index store, and concludes it "reads exact on a fresh store with simple puts, so it validates clean in isolation and only misleads on a real index" — which is why HNSW node counting uses a monotonic id counter instead. That matters here because the relationship estimate calls this helper ontable.indices[…], which is aRocksIndexStore extends RocksDatabase. The consequence is mis-sized range heuristics and condition ordering that can put a wide range ahead of a narrow indexedequals: plan quality, not correctness, and never wrong rows. It is Use storage-level statistical range estimates in the query planner #2163's trade, already merged onmain, and this PR carries it unchanged rather than re-litigating it — but if you want the backport scoped to the primary store only, this is the place to say so. Reverting is one line and reinstates the ~11s-per-condition scan.starts_with/between/sort/open-range estimate 1 — finite and positive, so the>> 4guard still functions; only the divisor and the subtraction produce values it cannot handle. Guarding them would need a representative count, not a floor, which is the hard problem above.estimatedEntryCount. That is wrong onmain, whereTable.tsreturns the value verbatim as the estimated pagination total for a condition-free query — an empty table would have answeredContent-Range: */1with zero rows, and the clamp below it only ever raises a total. v5.2 has no such consumer, but the branches should not disagree about what the accessor means. A unit test pins the count as 0.|| 1on the divisor,Math.max(…, 0)on the subtraction) reads inconsistent. It is deliberate:|| 1cannot catch a negative, and the two sites want different floors — 1 for a divisor, 0 for a cardinality.estimated_count >> 4is not a safe way to derive a threshold from an unbounded estimate. It wraps negative above 2³¹ (1e12 >> 4 === -45461248), and itsisNaNguard is dead code either way —NaN >> 4is0, so the check can never fire and a non-numeric estimate silently becomes threshold 0. Every pathology in this PR passes through that one expression. Review also surfaced a separate pre-existing defect in the same accumulator, which this PR does not touch: a zero-match condition followed by a full-scan comparator makes itNaN(0 * Infinity), andisFinite(NaN)being false then discards every estimate accumulated before it. The|| 1divisor is not involved — it isNaNfor any divisor. Fixing that, or clamping an intersection tomin(|A|,|B|)as set semantics require, is a planner-wide behavior change and wants its own PR onmain.ne nullsubtraction now mixes precisions. Its subtrahend,getValuesCount(null), is an exact index range count, while the minuend is now approximate — before this change both came from the same exact count. On a table after a bulk delete/reinsert cycle the estimate can fall below the live null count, sone nullclamps to 0 and the planner ranks it as the most selective condition. Worth knowing, though it is not made worse by the clamp: a negative sorted even more selective than 0. Same root as item 1.Verification
Unit tests
unitTests/resources/estimatedEntryCount.test.js— 6 tests. All 6 pass under RocksDB; underHARPER_STORAGE_ENGINE=lmdb5 pass and the RocksDB-specific one is pending. That test gates onstore instanceof RocksDatabaserather than ontypeof store.getEstimatedKeyCount, so a renamed or dropped binding method fails the guard loudly instead of silently skipping the one test that covers it. It asserts the returned estimate against an[N/2, 2N]band rather than comparing two live probes, so a background compaction between them cannot redden CI.Fails on base
Re-verified at the final head with
rm -rf distbefore each build, becausetscincremental will otherwise report a pass against stale output. The base isorigin/v5.2with only theexportkeyword added, so the export is not what fails and behavior is the only variable:estimatedEntryCount must not iterate the whole key spaceintersectionEstimatestays finiteInfinity !== 35ne nullestimate stays non-negative-500 !== 0Infinity !== 2004 of 6 fail on base. The other two pass there and should: the memo test is a regression guard, and the "count is reported as 0" test guards against the wrong fix rather than proving this one.
Dependency claim
v5.2 already pins rocksdb-js 2.8.0, so nothing moves. The method was additionally installed and exercised at 2.4.1, since the same change is going to v5.1:
RocksDatabase.prototype.getEstimatedKeyCountis a function there and its body is identical to 2.8.0's. The tombstone case that motivates the guards was measured on that build — 200 live rows plus 5,000 tombstones for never-written keys gives{estimated: 0, exact: 200}.That probe also rules out a
BigIntreturn (which would throw insideMath.max): its result was serialized withJSON.stringify, which throws on aBigIntand did not.Suites
test:unit:resources— 1702 passing, 0 failing.test:unit:main— 18 failing, all inglobalIsolation.test.jsandresolvePreload.test.js, which reproduce on the pristinemaincheckout too (macOS resolves the temp dir through/private/var), so CI is the authoritative run there.Not covered
The tests prove no
Infinityor negative escapes into a plan estimate. They do not prove the resulting plan is the better one on a churned table, and — per theDESIGN.mdnote above — the 2,000-row fixture is a fresh store with simple puts, which is precisely the profile the estimate reads accurately on. Neither the rewrite-inflated nor the tombstone-collapsed real-store case is covered; both need an end-to-end query against a churned table and are additive to this change.Complexity: medium
Review-Coverage: authored=claude; ran=gemini,cursor-composer,codex; adjudicated=domain; declined=cursor-grok; rounds=2 @ a098e64
Human-Review-Need: 3 (decisions: estimate-vs-exact-for-planning, apply-estimate-to-index-stores, zero-estimate-semantics, ne-null-clamp-to-zero, export-for-unit-test) @ a098e64