Use storage-level statistical range estimates in the query planner - #2163
Use storage-level statistical range estimates in the query planner#2163kriszyp wants to merge 4 commits into
Conversation
Range comparators (starts_with/prefix, between and friends, lt/le/gt/ge) have always estimated as fixed fractions of the table size (5%/10%/30%), which makes condition ordering — and #2147 count=estimated totals — wildly wrong for any real range. When the store provides rocksdb-js estimateCount ({ count, confidence }), estimateCondition now estimates the actual range the search would iterate (mirroring searchByIndex range construction), blended with the old fraction heuristic by the estimate confidence, so low-confidence estimates (block-granular tiny ranges, open-ended complement subtraction) degrade gracefully to the previous behavior. RocksIndexStore translates value-space bounds to its composite [indexedValue, primaryKey] keys ([value, MAXIMUM_KEY]), mirroring its getRange. estimatedEntryCount switches from an exact full-store getKeysCount scan (every 10s per store) to the O(1) estimate-num-keys read. The capability is feature-detected (typeof store.estimateCount), so behavior is unchanged until the rocksdb-js dependency ships HarperFast/rocksdb-js#778; the end-to-end tests self-skip until then.
- negated conditions now estimate the complement of their positive
estimate (root fix in estimateConditionForTable — also covers the
pre-existing negated-equals defect): a narrow negated range previously
looked highly selective, won the condition ordering, and executed as a
full scan
- the estimate path validates {count, confidence} shape and wraps the
native call in try/catch, so a future dependency bump (or a
concurrently closing store) degrades to the fraction heuristic instead
of NaN-poisoning plan ordering or failing the request
- over-length string bounds fall back (execution truncates at
MAX_SEARCH_KEY_LENGTH + filters, so the executed range is wider than
the estimable one)
- intersectionEstimate divisor floored at 1
- comment trims per review
The complement inversion still let a wide positive range make its negation look cheap while executing as a full scan. estimated_count feeds driving-condition ordering, and the codebase already encodes full-scan cost as Infinity for the filter-only comparators (contains/ends_with) — negated conditions (which always force needFullScan) now follow the same convention.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces statistical range estimation to the query planner, blending RocksDB's estimateCount with historical heuristics based on confidence. It also updates RocksDB's key count estimation to use the O(1) getEstimatedKeyCount method and adds comprehensive unit tests. The review feedback suggests guarding against nullish results from store.getStats() in estimatedEntryCount to prevent runtime errors, and preserving additional options (such as transactions or snapshots) when overriding estimateCount in RocksIndexStore.
| store.estimatedEntryCount = | ||
| store instanceof RocksDatabase ? store.getEstimatedKeyCount() : store.getStats().entryCount; |
There was a problem hiding this comment.
If store.getStats() returns nullish, accessing entryCount will throw a runtime error. Additionally, if store.getEstimatedKeyCount() or store.getStats()?.entryCount returns undefined, null, or NaN, store.estimatedEntryCount will be set to a non-finite value. We should guard against nullish results from getStats() and explicitly validate that the estimated count is a finite number, defaulting to 0 otherwise.
| store.estimatedEntryCount = | |
| store instanceof RocksDatabase ? store.getEstimatedKeyCount() : store.getStats().entryCount; | |
| const estimated = store instanceof RocksDatabase ? store.getEstimatedKeyCount() : store.getStats()?.entryCount; | |
| store.estimatedEntryCount = Number.isFinite(estimated) ? estimated : 0; |
References
- When accessing properties of a result from a method that might return nullish (e.g.,
backend.capabilities()), guard against nullish values to prevent runtime errors.
| (RocksIndexStore.prototype as any).estimateCount = function estimateCount(options?: any) { | ||
| let { start, end, exclusiveStart, inclusiveEnd } = options ?? {}; | ||
| if (exclusiveStart && start !== undefined) start = [start, MAXIMUM_KEY]; | ||
| if (inclusiveEnd && end !== undefined) end = [end, MAXIMUM_KEY]; | ||
| return (RocksDatabase.prototype as any).estimateCount.call(this, { start, end }); | ||
| }; |
There was a problem hiding this comment.
When overriding estimateCount for RocksIndexStore, only start and end are passed to the base estimateCount call. Any other options (such as transaction, snapshot, or future parameters) are stripped. It is safer to preserve the other options while removing exclusiveStart and inclusiveEnd (since their effects have already been manually applied to the composite boundaries).
(RocksIndexStore.prototype as any).estimateCount = function estimateCount(options?: any) {
let { start, end, exclusiveStart, inclusiveEnd } = options ?? {};
if (exclusiveStart && start !== undefined) start = [start, MAXIMUM_KEY];
if (inclusiveEnd && end !== undefined) end = [end, MAXIMUM_KEY];
const baseOptions = { ...options, start, end };
delete baseOptions.exclusiveStart;
delete baseOptions.inclusiveEnd;
return (RocksDatabase.prototype as any).estimateCount.call(this, baseOptions);
};![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // getStats is the LMDB fast path; for RocksDB, estimate-num-keys is O(1) where an exact | ||
| // getKeysCount() would iterate the entire store | ||
| store.estimatedEntryCount = | ||
| store instanceof RocksDatabase ? store.getEstimatedKeyCount() : store.getStats().entryCount; |
There was a problem hiding this comment.
What: store.getEstimatedKeyCount() is called unconditionally for every RocksDatabase instance, with no feature-detection, try/catch, or fallback — unlike the sibling estimateCount integration in this same PR (resources/search.ts:1120 guards with typeof store?.estimateCount !== 'function' and wraps the call in try/catch; resources/RocksIndexStore.ts:72 conditionally assigns the override so the capability probe doesn't lie).
Why it matters: estimatedEntryCount() is on the hot path for essentially every query-plan estimate against a RocksDB-backed table (not just the new range-estimation feature — it also feeds intersectionEstimate and every non-range branch's heuristic). If getEstimatedKeyCount is absent from the resolved @harperfast/rocksdb-js build (a platform-specific prebuild, a version skew, or simply if this method turns out not to exist on the pinned 2.7.1 the way estimateCount is claimed to need a future dependency bump), this throws a TypeError on effectively every RocksDB query, not a graceful degradation. That directly contradicts the PR's own stated design principle that "on the currently-pinned rocksdb-js the planner behaves exactly as before."
Suggested fix: Apply the same defensive pattern used for estimateCount elsewhere in this PR — feature-detect (typeof store.getEstimatedKeyCount === 'function') and fall back to the previous getKeysCount() when absent.
|
Reviewed. One finding posted inline on |
1 participant
Summary
Query-planner integration of rocksdb-js #778 — feat: statistical range key-count estimation.
Range comparators (
starts_with/prefix,betweenand thegele/gelt/gtlt/gtlefamily,lt/le/gt/ge) have always been estimated as fixed fractions of the table size (5%/10%/30%) — the code even said "just arbitrarily guess". That makes condition ordering, the adaptive filter→index switch, and #2147'scount=estimatedtotals wildly wrong for any real range. When the store providesestimateCount({ count, confidence }):estimateConditionnow estimates the actual range the search would iterate (range construction mirrorssearchByIndex's comparator switch, including the encoded prefix upper bound forstarts_withand[value, null]→[value, MAXIMUM_KEY]forprefix), blended with the old fraction heuristic by the estimate's confidence — a low-confidence estimate (block-granular tiny range, open-ended complement subtraction, degraded statistics) leans on the previous behavior instead of replacing it. Estimates never go below 1; primary-key ranges estimate against the primary store.Infinityat the root ofestimateConditionForTable, following the existing filter-only convention (contains/ends_with): the negated flag always forcesneedFullScan, so a (possibly narrow) positive-range estimate must never win the driving-condition ordering. This also fixes a pre-existing defect where negatedequalsestimated its (possibly tiny) positivegetValuesCount.RocksIndexStoregains anestimateCountoverride translating value-space bounds to its composite[indexedValue, primaryKey]keys ([value, MAXIMUM_KEY]), mirroring itsgetRange— assigned conditionally sotypeof store.estimateCount === 'function'remains a capability probe.estimatedEntryCountswitches from an exactgetKeysCount()full-store iteration (re-run every 10 seconds per store) to the O(1)getEstimatedKeyCount()property read — this needs no dependency bump and pays off immediately on large tables. TheintersectionEstimatedivisor is floored at 1.Everything is feature-detected and defensive: on the currently-pinned rocksdb-js the planner behaves exactly as before (verified against the stock dependency), and the estimate path validates the
{count, confidence}shape and try/catches the native call, so a future caret-activated dependency (or a concurrently closing store) degrades to the fraction heuristic rather than NaN-poisoning plan ordering or failing the request. Bounds longer thanMAX_SEARCH_KEY_LENGTHalso fall back — execution truncates + filters there, so the executed range is wider than the estimable one.For the human reviewer
round(confidence × estimate + (1 − confidence) × fraction-heuristic). At confidence 1 the statistical estimate wins outright; at 0 the old behavior is preserved. A hard threshold was rejected as a cliff.estimated_countsemantics: for negated conditions it is now execution-cost ordering (Infinity), not result cardinality — consistent with the other full-scan comparators. If feat(rest): total-count pagination viaPrefer: count=(Content-Range) #2147 later wants a cardinality for negated queries, complement arithmetic can be added on its side.estimatedEntryCountsemantics shift slightly and this is the one behavior change live before the dependency bump:estimate-num-keysskews high on overwrite/delete-heavy data until compaction, where the old exact scan did not. All consumers are relative-ordering or explicitly-estimated paths; the accuracy-per-cost trade is deliberate (review kept it as a noted minor).sort,equals,in,nebranches untouched —equalsselectivity still uses the exact per-valuegetValuesCount.{count, confidence}contract.Verification
unitTests/resources/estimateRangeCondition.test.js: 13 stub-store tests — dispatch, per-comparator range construction (asserting the exact ranges passed to the store), the confidence blend (1 / 0 / 0.5), primary-key routing, capability-absent fallback, the ≥1 floor, negated→Infinity(range and equals), malformed-shape fallback (bare number, NaN, missing/out-of-range confidence, null), thrown-native-call fallback, and over-length-bound fallback. These run on the stock dependency. Plus 3 end-to-end tests against real tables with secondary indexes (capability-gated): width-orderedbetweenestimates, real prefix ranges forstarts_with, ordered open-range tails.unitTests/resources/conditionsArrayMutation.test.js(planner-adjacent) passes. Full unit gates rely on CI (local runs hit shared-lock contention per prior experience).tscbuild clean; oxlint clean; prettier applied to changed files.Prefer: count=(Content-Range) #2147'scount=estimatedtotals improve automatically once both PRs land plus the dependency bump. No user-facing API/config change → no documentation PR needed.Review coverage
Generated by Claude (Fable 5). Cross-model pre-push review via
prepush-review.mjs, three rounds:d05e194): Codex (graded) + Harper-domain adjudication — Gemini timed out, cursor-composer failed (output format), grok pruned. Two majors fixed in round 2: negated ranges receiving their positive-range estimate while execution full-scans, and blind trust in an unshipped dependency shape (caret-activation NaN risk). Minors fixed: no try/catch around the native call,MAX_SEARCH_KEY_LENGTHtruncation divergence forstarts_with, divisor floor.38052b9): Codex (Gemini failed again). Confirmed the guards; kept a major on the complement inversion (a wide positive range still made its negation look cheap) — fixed in round 3 by adopting theInfinityfull-scan convention.Accepted, not changed (with rationale): the per-condition native probe on the planning path (bounded, ~10µs, memoized per condition);
estimate-num-keyscardinality skew on overwrite-heavy tables (deliberate O(1) trade, floored divisor, relative-ordering consumers).Human-Review-Need: 3 @ f917da4