perf(core): base-table-first point lookups for get_by_id/get_blob - #173
Merged
Conversation
Single-record reads unconditionally opened every pending flushed WAL generation (base ∪ ALL generations), so a point lookup paid O(N) object-store opens even when the row was merged into the base table long ago — tens of seconds on a high-write experiment with hundreds/thousands of pending generations, plus transient 500s when a concurrent merge deleted a generation between the manifest snapshot and the open. Query the immutable base table first and return on a hit, opening zero generations for the common already-merged case; only a base miss falls back to the WAL. This is a pure optimization, not a semantic change: rollout rows are immutable and an id is never re-appended, so a row in the base table is identical to any (absent) WAL copy, and a WAL-only row is still found by the fallback — the fetchability invariant holds. - get_by_id: base-first via new get_by_id_source(id, ListSource); the default (All) is base-first-then-WAL. Fragments/Wal select a single source, mirroring #170's list-by-source. - get_blob: query the base table first (was: last, after scanning every generation newest-first), then fall back to the flushed generations opened with bounded concurrency (DEFAULT_OBSERVE_CONCURRENCY), first hit wins. Because an id lives in exactly one place, the parallel fallback needs no ordering. - get_blob fallback tolerates a generation concurrently merged+deleted (is_not_found_error -> skip) instead of failing the request, removing the transient 500 under concurrent auto-merge. - Tests: un-merged rows found via fallback; merged rows found via base; Fragments/Wal source selection; immutability contract guard. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
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
1 participant
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.
Summary
Point lookups (
get_by_id,get_blob) unconditionally unioned every pending WALgeneration, opening each one from object store. Under a large WAL backlog (700+
generations seen) this made single-record fetches take tens of seconds, and produced
intermittent 500s from a TOCTOU race with background merge (reader snapshots the shard
manifest, a concurrent merge drains+deletes a generation, the reader then opens the now
-deleted dir →
NotFound).Because rollout rows are immutable (an id is appended exactly once and never
overwritten — a row lives in the base table or a single generation, never both), we
can read the base table first and only fall back to the WAL when it misses.
Changes (core only —
rollout_store.rs)get_by_id→ base-first. Newget_by_id_source(id, source)+ privatescan_one_by_id.AllscansFragments(base, zero generation opens) first andreturns on hit; only on a miss does it scan
Wal. Publicget_by_iddelegates toAll, preserving the fetchability invariant (un-merged rows are still found).get_blob→ base-first + bounded-parallel, NotFound-tolerant fallback. Queries thebase dataset first, then fans out over flushed generations with
buffer_unordered(DEFAULT_OBSERVE_CONCURRENCY=16)— first hit wins, and generationsthat were concurrently merged/deleted (
is_not_found_error) are skipped instead oferroring. The concurrency cap avoids exhausting the object-store connection pool.
is_not_found_errorhelper.Merged rows: O(1) base read, zero generation opens. Un-merged rows: correct via the
fallback. No change to durability/semantics or to the immutability contract.
Tests
point_lookup_base_first_finds_unmerged_and_merged_rowspoint_lookup_immutability_contract_returns_base_versioncargo fmt --all -- --check,cargo clippy --workspace --all-targets -D warningsclean.cargo test -p lance-context-corelib tests pass (incl. the 2 new).get_by_id+get_blobresolve throughthe WAL fallback; post-merge base-first hit works.
Known unrelated failure
serial_merge_deletes_merged_generation_dirsfails on this branch and on a cleanorigin/main(appends=30 row_count=32) — pre-existing/flaky, does not touch thepoint-lookup path.
Scope
P0/P1 only. P2 — making lance's
LsmScannertolerate concurrently-merged generations forthe
?source=wal/?source=alllist path — is proposed separately and not includedhere.