perf(encoding): compact chunk index for sparse cached page state - #7947
perf(encoding): compact chunk index for sparse cached page state#7947Ali2Arslan wants to merge 2 commits into
Conversation
Follow-up to lance-format#7651. The sparse structural scheduler cached a `Vec<ChunkMeta>` (num_values, byte size, byte offset per chunk) plus a separate `Arc<[u64]>` of cumulative value offsets -- ~32 bytes/chunk, all derivable from two cumulative arrays. Sparse value chunks are themselves mini-blocks, so this reuses the `MiniBlockChunkIndex` from lance-format#7651: - byte ranges come from the existing `byte_starts` prefix sums (u32 when the value buffer fits), and - value->chunk lookups use `RowMapping`, collapsing to arithmetic (`UniformFlat`) when non-last chunks share a value count, else an explicit value-starts array. This drops the redundant per-chunk byte offsets and value offsets, taking the cached state from ~32 bytes/chunk to 4-8 bytes/chunk while preserving all metadata validation. `ChunkMeta`, now unused, is removed. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughSparse primitive decoding replaces cached chunk metadata and cumulative offsets with ChangesCompact chunk index migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PageInitialization
participant build_value_chunk_index
participant SparseStructuralCacheableState
participant schedule_ranges
participant MiniBlockChunkIndex
participant append_value_range
PageInitialization->>build_value_chunk_index: metadata bytes
build_value_chunk_index->>SparseStructuralCacheableState: constructed value_index
PageInitialization->>schedule_ranges: cached value_index
schedule_ranges->>MiniBlockChunkIndex: find_chunk for visible range
schedule_ranges->>append_value_range: selected chunk range
append_value_range->>MiniBlockChunkIndex: first_row and items_in_chunk
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/lance-encoding/src/encodings/logical/primitive/sparse.rs (1)
5654-5670: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider covering the enlarged-last-chunk case. The test exercises
uniform_flatandflat, but not aUniformFlatpage whose last chunk holds more values than the non-last chunks (e.g.[(_, 2), (_, 4)]), which is where the arithmetic row→chunk mapping is most fragile. Adding a case that both initializes and resolves a value inside that last chunk would guard the concern raised on thebuild_value_chunk_indexuniform selection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-encoding/src/encodings/logical/primitive/sparse.rs` around lines 5654 - 5670, Extend value_chunk_index_stays_compact_across_row_mappings to cover a UniformFlat layout with a larger final chunk, such as value counts 2 then 4. Initialize this case through init_value_index and resolve an index targeting a value in the final chunk, verifying the row-to-chunk mapping succeeds and remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rust/lance-encoding/src/encodings/logical/primitive/sparse.rs`:
- Around line 5654-5670: Extend
value_chunk_index_stays_compact_across_row_mappings to cover a UniformFlat
layout with a larger final chunk, such as value counts 2 then 4. Initialize this
case through init_value_index and resolve an index targeting a value in the
final chunk, verifying the row-to-chunk mapping succeeds and remains correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 12680847-b85e-4db3-b13c-561a3cd669cd
📒 Files selected for processing (2)
rust/lance-encoding/src/encodings/logical/primitive.rsrust/lance-encoding/src/encodings/logical/primitive/sparse.rs
💤 Files with no reviewable changes (1)
- rust/lance-encoding/src/encodings/logical/primitive.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Review the sparse chunk-index migration against total cache accounting, not just heap children. Box nested-only row metadata so flat indices stay small, retain trailer bits without Arrow buffer header overhead, and remove redundant page and chunk counts. Build final prefix sums by rescanning validated metadata instead of holding two temporary per-chunk vectors. Derive complete DeepSizeOf accounting for the sparse structural plan and extend tests across non-uniform reads, enlarged final chunks, empty/single/multi-chunk memory, and all row-mapping variants. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
Follow-up to #7651. The sparse structural scheduler cached a
Vec<ChunkMeta>(value count, byte size, byte offset) plus a separate cumulative-value-offset array — about 32 bytes per chunk, with most fields derivable.Sparse value chunks are mini-blocks, so this replaces that state with the compact
MiniBlockChunkIndexintroduced in #7651:PrefixSums(u32when the value buffer fits);UniformFlatmapping;Flatvalue-prefix array;Box<[u8]>, avoiding an unnecessary Arrow buffer header.Metadata is validated once and then rescanned to build only the final prefix sums, avoiding two temporary
Vec<u64>allocations per page. The cached state also no longer duplicatesnum_visible_items, andDeepSizeOfnow includes the sparse plan's layer-vector storage and nested allocations.Memory behavior
The tests now use
deep_size_of()(the same total inline + heap accounting used by the cache), rather than checking heap children alone:UniformFlatandFlatpages are smaller than the legacy representation;Testing
cargo test -p lance-encoding --lib sparse— 55 passedcargo clippy -p lance-encoding --tests --benches -- -D warningscargo fmt --all --checkMade with Cursor