perf(index): reduce describe indices fragment scans#6958
Conversation
|
@Xuanwo @BubbleCal @jiaoew1991 Hi, could you please help review this PR when free? Thanks! |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| for fragment in dataset.iter_fragments() { | ||
| fragment_rows.insert( | ||
| fragment.id as u32, | ||
| fragment_logical_rows_from_metadata(fragment)?, |
There was a problem hiding this comment.
The ? operator propagates Error::internal if any fragment in the manifest lacks complete row metadata (physical_rows or num_deleted_rows). This iterates all fragments upfront, including those never referenced by any index bitmap. The old code only called fast_logical_rows() for fragments that appeared in the bitmap, so fragments with incomplete metadata were tolerated as long as they were not indexed.
There was a problem hiding this comment.
Realistically, any modern Lance dataset will have physical_rows or num_deleted_rows. Generally, though, we try to use the async version to be backwards compatible. If it's not too much trouble, I'd prefer we kept with that.
This iterates all fragments upfront, including those never referenced by any index bitmap. The old code only called
fast_logical_rows()for fragments that appeared in the bitmap, so fragments with incomplete metadata were tolerated as long as they were not indexed.
TBH I don't this this is a huge deal. Realistically, you will have indexes covering most if not all fragments, so you'll use all or most of the fragment row counts. I think the speed benefit of collecting them into a contiguous allocation (versus computing on demand) might outweigh the cost of slight over-computation.
There was a problem hiding this comment.
We could keep this is as-is if we do the error message change I suggest in: https://github.com/lance-format/lance/pull/6958/changes#r3314011721
There was a problem hiding this comment.
@wjones127 Thanks for your valuable suggestion. Have applied.
Co-authored-by: Will Jones <[email protected]>
|
Hi, @wjones127 @LuciferYang After apply this optimization, we reduce describe_indices cost time significantly from 400+ second to 0.7 seconds within 500B rows lance dataset . I think this is a very useful PR. |
|
|
||
| let desc = &descriptions[0]; | ||
| assert_eq!(desc.name(), "test_idx"); | ||
| assert_eq!(desc.rows_indexed(), 4); |
There was a problem hiding this comment.
The single assert_eq!(desc.rows_indexed(), 4) exercises one fragment, no deletions, no missing-bitmap entries. Per the project standard ("Include multi-fragment scenarios for dataset operations"), at least one of the following should be covered, ideally in a separate #[tokio::test] to keep the existing test focused:
- Multiple appended fragments — verifies the HashMap aggregation sums across fragments.
- A fragment with deletions — verifies
num_rows()returnsphysical_rows - num_deleted_rows, not raw physical rows. - A stale segment bitmap (e.g., after compaction) referencing a fragment that no longer exists in the manifest — exercises the new
missing_fragment_refsbranch.
There was a problem hiding this comment.
@LuciferYang Thanks for suggestions. Will add unit tests laterly.
Co-authored-by: Cursor <[email protected]>
|
@LuciferYang @wjones127 Have added unit tests. please check again. Thanks. |
Summary
Optimize
describe_indices()row-count aggregation by building a singlefragment_id -> logical_rowsmap from manifest metadata and then iterating each segment'sRoaringBitmapdirectly.This avoids calling
Dataset::get_fragments()inside the per-segment loop and adds trace-level instrumentation for the number of dataset fragments, segment fragment references, missing fragment references, and computed indexed rows.Results
We use the same test envirnment to compare the optimize result. See below:
before optimization (425 seconds):
after optimization(0.701 seconds)
Why
The previous implementation computed
rows_indexedby looping over every dataset fragment for every index segment:That makes the work proportional to
num_index_segments * total_dataset_fragments. On a large table, for example 10,000 index segments and 1,000,000 fragments, this can turn a metadata-only description into billions of membership checks even when each segment covers only a small fragment bitmap.There was also avoidable allocation in the inner loop.
Dataset::get_fragments()clones the dataset into anArcand clones each manifest fragment into aFileFragmentwrapper. Because it was called once per segment, those clones and wrappers were rebuilt repeatedly while describing one logical index.The new path builds fragment row metadata once with
Dataset::iter_fragments()and then performs work proportional to:For lance-ray style scheduling, this keeps the driver-side metadata planning step much lighter before slicing work across workers.
Validation
cargo fmt --allcargo test -p lance test_describe_indices_total_size -- --nocapturecargo clippy --all --tests --benches -- -D warningsgit diff --check