Conversation
Collaborator
|
There is already a #7389 which seems to address the same issue? |
Author
We hadn't looked at #7389 when making this change. It does target the same issue (#7198), and we implemented ours to address performance and OOM problems during index optimisation that we have been hitting. In a way this is the same PR re-implemented on top of #8840, which rewrote the Bitmap build around a sorted stream and a bounded writer. Core differences, as far as I can tell:
This PR does not touch the legacy shard machinery that #7389 removes. I may be biased, since we have been running this change in production, but I believe it carries #7389 forward onto the current code, and the memory profile is the part we cannot do without. |
…taset
`merge_scalar_indices` lists only `IndexType::BTree` and `IndexType::NGram`
in `has_segment_merge_primitive`, so a Bitmap index with more than one
selected segment falls through to `rebuild_scalar_segment` -- a full rescan
and retrain over every covered fragment. Bitmap declares
`TrainingCriteria::new(TrainingOrdering::Values)`, so that rescan is wrapped
in a blocking spilling `SortExec` against `FairSpillPool(LANCE_MEM_POOL_SIZE)`.
On a large low-cardinality column that means sorting every row of the column
to produce an index of a few megabytes, and failing outright once the pool
is exhausted.
Wire Bitmap into the same N:1 segment-merge path BTree uses. The old side of
`build_index_map` becomes `OldSegments`: K source segments presented as one
ascending-key stream of postings, a min-heap over each segment's in-memory
`index_map` keys, with each segment's bitmaps read one key at a time through
`load_bitmap`. `merge_index_maps` is re-expressed over the same type, so the
single-segment update path, the LabelList merge and the new
`BitmapIndex::merge_segments` share one merge-join. Two choices are
load-bearing:
- Keys come from `index_map`, a `BTreeMap`, so key order does not depend on
the on-disk row order. Bitmap lookup files are not reliably key-sorted --
an update appends an old-only null row last, and LabelList files written
before spill-based builds are unsorted -- so a merge that read the files
sequentially and assumed sorted input would silently drop postings.
- `load_bitmap` is the accessor that applies fragment-reuse remapping.
Reading the lookup file's raw bytes would bypass it and emit row
addresses pointing at fragments compaction has already retired, which
under deferred index remapping is silent data loss.
Peak memory on the old side is one posting per segment for the current key
plus the union being emitted, on top of each segment's resident `index_map`.
Per-segment `OldIndexDataFilter`s are applied to each source posting, so
compaction-retired fragments and stable-row-id rewrites are dropped exactly
as on the BTree path, and a filter that keeps nothing skips the segment's
bitmap reads (its `index_map` is still loaded when the segment is opened).
`Dataset::merge_existing_index_segments` now routes bitmap groups through the
same primitive, which gives it the per-segment filtering it lacked.
`merge_bitmap_indices`, which it replaces, is deprecated and forwards to
`merge_segments` with no new data and no filters.
`BitmapIndex::merge_segments` promises that its output is the union of its filtered inputs, and nothing asserted that directly. The existing tests pin hand-built shapes; this covers the contract itself. 200 fixed seeds, each building 1-4 segments over a 5-value vocabulary and a 4-fragment address space so keys and row addresses collide across segments, every `OldIndexDataFilter` arm including ones that keep nothing, and nulls on either side. `oracle_keeps` reimplements `retain_old_rows` rather than calling it, so the oracle is independent of the code under test. Assertions read `index_map`/`null_map` rather than `search`, comparing what the merge wrote instead of how a query later resolves a row that is both null and non-null. A key whose rows are all filtered out may still be materialised with an empty bitmap, so only a key outside the vocabulary counts as fabricated. Runs in about a second.
Every other Bitmap merge test goes through `optimize_indices` and builds one segment per fragment, so the distributed-build entry point had no coverage and neither did a segment covering more than one fragment -- which is the only way a per-segment filter is a strict subset of what the segment holds. `test_bitmap_merge_existing_index_segments_multi_fragment` commits two segments over four fragments, two fragments each, deletes one row from each segment's coverage, then merges and opens the result directly. Stable row ids make the filter an exact row-id allow-list, so the deletions reach it instead of being masked at scan time. `test_optimize_bitmap_drops_stale_rows_across_segments_after_update` now asserts one segment remains. It selects two today, but nothing pinned that, so a change to `select_segments_to_merge` could have quietly moved it onto the single-segment `update` path while it kept its name.
`build_index_map` groups rows into runs by comparing each value with the previous one only, so a value that reappears after another value opens a second run for a key already written. `BitmapIndex::load` keys `index_map` by value and keeps the last file offset, so the earlier run's row set was dropped with no error or log line -- a `debug_assert!` caught it in debug builds and nothing did in release builds. Nulls have the same exposure through `null_map`, which takes the last null entry in the file. Check the ordering at each value change and return `Error::invalid_input` naming both keys, in release builds too. The cost is one `OrderableScalarValue` comparison and two scalar clones per distinct value -- per run, not per row -- against a build that already serializes a bitmap per run; unsorted input otherwise silently produces a corrupt index. `BitmapIndex::merge_segments` and `BitmapIndexPlugin::train_bitmap_index` take a caller-supplied stream and validated everything about it except the ordering the build depends on. Only the non-null keys have to ascend. Nulls are collected into `null_map` rather than merge-joined by value, so one null run is correct wherever it falls, and both `asc_nulls_first` and `asc_nulls_last` input stay valid; a second null run is what gets rejected. Cover nulls end to end as well. Every `optimize_indices` test used a non-nullable column, so no null reached the scanner's ordering, the per-segment filters or the commit. `test_optimize_bitmap_multi_segment_merge_keeps_nulls` consolidates three segments plus an unindexed tail over a column that is null every fourth row, and checks `IS NULL` and `IS NOT NULL` against the value row sets.
`OldSegments` read each source row set through `load_bitmap`, which caches what it reads. A merge touches every key of every source segment exactly once, and the commit that follows retires those segments, so every entry it inserted was dead on arrival while evicting entries for indices that still exist. The cache holds up to 6 GiB by default, so a wide consolidation can displace a lot. Each row set was also copied three times: deserialized, cloned for the cache insert, then cloned again to get something the filter could run on in place. Split the read at the cache boundary. `read_bitmap_at` does the read, deserialize and fragment-reuse remap; `load_bitmap` keeps its null, absent-key and cache handling around it, unchanged for `search` and for `remap_index_map`. `read_bitmap_uncached` gives the merge an owned row set with no cache interaction, which `OldSegments` then filters in place, leaving one copy instead of three. The same path serves the single-segment `update` and the LabelList merge, whose sources are retired just the same. Going through `load_bitmap` was never gratuitous -- it is what applies the fragment-reuse remap, and reading the file directly would resurrect row addresses pointing at retired fragments. `read_bitmap_at` keeps that remap, so only the caching is bypassed.
An old-data filter or a remap can leave a key with no rows. Only the k-way merge in `merge_index_maps` dropped such a key; the update path wrote it with a zero-row bitmap, and `remap_index_map` did so deliberately, to keep one output row per source key. `BitmapIndex::load` builds `index_map` from the keys column alone, so a written empty key comes back indistinguishable from a live one. Nothing prunes it, and every later merge re-reads and re-emits it, so a column whose values stop occurring -- retired by compaction, deleted, or updated away -- accumulated one permanent directory entry per value it ever held. The resident key directory is what bounds a bitmap index, so the cost lands on the one scarce resource. Skip the entry in `BitmapBatchWriter::emit` instead, the single choke point for every write path, and drop the per-path guards that duplicated it. Absent and present-but-empty are query-equivalent -- both match no rows -- so nothing observable changes for readers, while dropping saves an entry, a bitmap load and a heap step per emptied key in every later merge, and gives every path one rule instead of two. Both filter variants drop emptied fragments, so an `is_empty` set really holds no rows. An all-filtered merge now writes a row-less file, which loads correctly because the file schema carries the value type independently. This also realigns `statistics()`, which counts `index_map` plus the null row, with the file's `num_bitmaps` metadata, which counts entries written.
amunra
marked this pull request as ready for review
September 16, 2026 14:31
amunra
force-pushed
the
upstream/bitmap-merge-segments
branch
from
September 16, 2026 14:31
df26dd0 to
ee8df23
Compare
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
the
K-changes
…ap ordering check `check_run_order` returned early whenever the previous run was null, so the non-null key before that run was never compared with the one after it. Input such as `["a", NULL, "a"]` passed the check and was written as two `"a"` entries, of which `BitmapIndex::load` keeps only the last, silently losing the first posting. Track the most recent non-null key in `build_index_map` and compare each new non-null key against that rather than against the immediately preceding run. A single null run may still lead or trail; the non-null sequence on either side of it is now checked as one sequence. Two cases cover it: a value reappearing across the null run and a value descending across it.
lance-gatekeeper
Bot
removed
the
K-changes
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The previous ordering blocker is fixed by carrying the last non-null key across a null run, and the repeated and descending regression cases now pass. The bounded k-way segment merge preserves bitmap semantics while avoiding the full dataset rescan and remains the preferable solution.
lance-gatekeeper
Bot
added
the
K-approved
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
3 participants
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
merge_scalar_indiceslists onlyIndexType::BTreeandIndexType::NGraminhas_segment_merge_primitive, so a Bitmap index with more than one selected segment falls through torebuild_scalar_segment: a full rescan and retrain over every covered fragment. Bitmap trains onTrainingOrdering::Values, so that rescan is wrapped in a blocking, spillingSortExecagainstFairSpillPool(LANCE_MEM_POOL_SIZE). On a large low-cardinality column this sorts every row of the column to produce an index of a few megabytes, and fails outright once the pool is exhausted.The k-way merge that
merge_bitmap_indicesalready runs overindex_mapkeys was not reachable fromoptimize_indices, could not take a new-data stream, and applied a single filter to every source.Dataset::merge_existing_index_segmentsfor Bitmap applied no old-data filter at all.Change
Bitmap joins the N:1 segment-merge path BTree uses.
OldSegmentspresents K source segments as one ascending-key stream of filtered postings: a min-heap over each segment's in-memoryindex_mapkeys (borrowed, not cloned), with each segment's bitmaps read one key at a time.build_index_maptakesVec<OldSegment>instead of a single optional old index, so the single-segmentupdatepath, the LabelList merge (merge_index_maps) and the newBitmapIndex::merge_segmentsshare one merge-join.merge_bitmap_indicesis deprecated and forwards tomerge_segments;merge_existing_index_segmentsnow goes throughbuild_per_segment_filtersand the shared primitive.index_map(aBTreeMap), because bitmap lookup files are not reliably key-sorted on disk (an update appends an old-only null row last, and LabelList files written before spill-based builds are unsorted). Bitmaps are read through the accessor that applies fragment-reuse remapping, because reading raw file bytes would emit row addresses that point at fragments compaction already retired, which under deferred index remapping is silent data loss.OldIndexDataFilters drop compaction-retired fragments and stable-row-id rewrites exactly as on the BTree path; for a segment whose filter keeps nothing, none of its bitmaps are read (itsindex_mapis still loaded when the segment is opened).read_bitmap_uncached): every source entry is read once and the sources are retired by the following commit, so caching them only evicted live entries. The fragment-reuse remap is kept; only the caching is skipped.Error::invalid_inputin release builds, replacing adebug_assert!. A value that reappears after another value reopens a run for a key already written, andBitmapIndex::loadkeeps only the last file offset per key, so the earlier row set vanished silently. The cost is one comparison and two scalar clones per distinct value, not per row. Nulls may lead or trail as a single run.BitmapBatchWriter::emit, which also coversremap_index_map, which previously wrote such keys deliberately. Absent and present-but-empty are query-equivalent, so nothing observable changes for readers; dropping saves an entry in the resident key directory, a bitmap load and a heap step per emptied key in every later merge, and gives every path one rule instead of two.Peak memory on the old side of a merge is one posting per segment for the current key plus the union being written, on top of each segment's resident
index_map. On a synthetic 500M-row dataset with 26 delta segments, consolidation throughoptimize_indiceswent from 36-40 minutes to about 2.5 minutes and stayed flat across 8, 16 and 24 GiB memory pools, where the rescan path failed with memory-pool exhaustion at 8 GiB.Public API
One addition and one deprecation,
rust/lance-index/src/scalar/bitmap.rs:BitmapIndex::merge_segmentspub async fn; the k-way merge this PR addsmerge_bitmap_indices#[deprecated(note = "use BitmapIndex::merge_segments")]The deprecated function keeps its signature and behaviour, forwarding to
merge_segmentswith no old-data filters and an empty new-data stream. It is not removed.Tests
lance-index:test_bitmap_merge_k_segments{,_without_new_data,_with_new_nulls,_boolean_keys},test_bitmap_merge_matches_brute_force_union(200 seeds against an independent oracle),test_bitmap_merge_does_not_cache_source_bitmaps,test_bitmap_build_rejects_unsorted_input(rstest),test_bitmap_merge_drops_emptied_keys;test_bitmap_segment_merge_matches_single_buildandtest_bitmap_remap_matches_materialized_pathadapted.lance:test_optimize_bitmap_multi_segment_merge_consolidates,test_optimize_bitmap_wide_consolidation(26 segments),test_optimize_bitmap_multi_segment_merge_keeps_nulls,test_optimize_bitmap_merge_remaps_deferred_compaction,test_optimize_bitmap_drops_stale_rows_across_segments_after_update,test_bitmap_merge_existing_index_segments_multi_fragment.Compatibility
No on-disk format change and no index version bump: the merged file is a regular
bitmap_page_lookup.lancethat existing readers load as before. Files written by earlier versions merge the same way, because the merge is driven by the loadedindex_map, not by on-disk row order.lance_index::scalar::bitmap::merge_bitmap_indiceskeeps its signature and is marked#[deprecated]; it forwards toBitmapIndex::merge_segmentswith no new data and no filters and keeps its progress stages.BitmapIndex::merge_segmentsis the replacement, with per-segment filters and a new-data stream.Tracking: Ported from rerun-io#62.