fix: a merge extends a stale key BTree, not just builds a missing one - #286
Merged
Merged
Conversation
lance-format#282 builds the key BTree before the delete-only merge_insert when the base has none. Present is not enough: every merge and compaction appends fragments the index does not cover, and merge_insert hash-joins exactly those. A 2.2 GB store whose BTree covered 1 of 65 fragments took a worker from 5 to 32 GiB on a 37 MB merge and OOMKilled 19 of 20 at once; after one optimize_indices the same merge on the same worker peaked at 3 GiB. The worker now does what the master already does before fan-out (lance-format#278): build the index if missing, otherwise extend it over the unindexed fragments. Counted as rollout_merge_index_extended_on_demand_total. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
append() adds one index delta per merge. Compaction refuses to bin fragments covered by different index-delta sets together, so with a delta per merge every fragment sat in its own bin and nothing ever coalesced (three compaction tests caught it). merge(1) folds the new fragments into the existing delta, so the index stays a single delta. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Follow-up commit: |
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.
Problem
#282 builds the key BTree before the delete-only
merge_insertwhen the base has no index. But an index that exists and covers only some fragments is just as bad:merge_inserthash-joins every fragment the index does not cover.Production, 00:55 UTC:
bp-prod2rubric-…-ghcp-100(2.2 GB base, 65 fragments, BTree built when it had 5) — a merge of 64 generations totalling 37 MB took a worker from 5 to 32 GiB and OOMKilled 19/20 workers that merged it concurrently. Reproduced deterministically on one worker (5 → 20+ GiB → killed). After a singleoptimize_indicesthe identical merge on the identical worker peaked at 3 GiB and finished in 39 s.Fix
In
merge_prepared_batches: if the base has fragments and no BTree, build it (unchanged); otherwise callextend_key_btree_indexso the index covers everything appended since — the same thing the master does before fan-out (#278), now also on the worker's own merge path (timer, manual route). New counterrollout_merge_index_extended_on_demand_total.Verification
merge_extends_a_stale_id_btree_over_new_fragments: after four merges at most one fragment (the one just appended) is uncovered.extend_id_btree_index_covers_fragments_added_since_buildupdated to the new invariant (1 uncovered after two merges, not 2).🤖 Generated with Claude Code