perf(encoding): auto-select sparse structural pages#7756
Conversation
📝 WalkthroughWalkthroughAdds Lance 2.3+ sparse structural page encoding, automatic layout selection, sparse value and metadata serialization, sparse-aware rep/def decoding, compatibility validation, tests, and format documentation. ChangesSparse structural page layout
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
ed0ce68 to
d7cab0f
Compare
d7cab0f to
4c1eb4f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/lance-encoding/src/encodings/logical/primitive/sparse/writer.rs (1)
225-226: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePre-size
non_empty_positionsandcounts.Both vectors grow across the
0..num_slotsloop, so their capacity is estimable. Prefer over-estimating withnum_slotsto avoid repeated reallocations.As per coding guidelines: "Use `Vec::with_capacity()` when size is known or estimable, and prefer over-estimating capacity to multiple reallocations."♻️ Proposed change
- let mut non_empty_positions = Vec::new(); - let mut counts = Vec::new(); + let mut non_empty_positions = Vec::with_capacity(num_slots); + let mut counts = Vec::with_capacity(num_slots);🤖 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/writer.rs` around lines 225 - 226, Initialize non_empty_positions and counts with Vec::with_capacity(num_slots) before the 0..num_slots loop, preserving their existing element types and subsequent behavior.Source: Coding guidelines
🤖 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/writer.rs`:
- Around line 225-226: Initialize non_empty_positions and counts with
Vec::with_capacity(num_slots) before the 0..num_slots loop, preserving their
existing element types and subsequent behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 20b59d15-985b-42d7-bf23-8a5944e1606a
📒 Files selected for processing (5)
docs/src/format/file/encoding.mdrust/lance-encoding/src/encodings/logical/primitive.rsrust/lance-encoding/src/encodings/logical/primitive/layout.rsrust/lance-encoding/src/encodings/logical/primitive/sparse/writer.rsrust/lance-encoding/src/repdef.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Part of #7750
Summary
This is the policy/performance layer that turns the already-readable and explicitly-writable sparse format on automatically only for over-budget eligible pages.
Integration
mainbase before the policy commits in this PR.MiniBlockRepDefBudgetnaming.100M-row S3 benchmark
Environment:
r7i.8xlargeinus-east-2b, Lance V2.3, 100 files of 1,000,000 rows, 65,536-row write batches, and 1,024-row takes. The final committed harness is at66c936391aa46bfcd14f7ac1c820878da68fcc0bonxuanwo/sparse-stack-5-auto-sparse-bench-final, based on pre-rebase PR5 headed0ce68d7f4cceafe6a249c912f7f1da8c4fecd9. The rebase consolidates #7754 and resolves #7751 naming without changing the auto-selection or dense-fallback policy; these measurements remain attributed to that exact benchmarked head and are not relabeled as current-head results.Cases:
hnsw:List<UInt32>; the first 1/7 of rows (14,285,714 of 100M) contain 32 consecutive values, followed by an empty tail.uniform:List<UInt32>; one singleton non-empty row every 10,000 rows.deep:Struct<events: List<Struct<id: Int32, tags: List<Int32>, pair: FixedSizeList<Int32; 2>>>>; non-empty every 4,096 rows with nullable top struct, event list, event struct, id, tags, pair, and pair items.coldis the first operation on a newly openedDataset;warmis the immediate repeat on the sameDataset. Times are milliseconds. Bytes are total dataset object bytes. Every dataset has 102 objects, including 100 Lance data files.deep/full-zipwrote all 100M rows and produced the reported size/page/object counts, then its first full scan panicked atprimitive.rs:2588by unwrapping a missing fixed full-zip decode task. Sparse and mini-block completed all reads. A separate 200K-row check at the exact PR4 base SHAda06b79a7f5eda4f33e2d80990c46766095ba383independently confirmed that this nested-nulldeep/full-zipread was already broken before this PR, returningStructural validity has 6 entries for an array with 7 values. The precise failure site varies with this decoder path, but explicit full-zip never enters PR5's automatic sparse candidate closure, so this is not an auto-sparse regression. The policy deliberately does not disguise that behavior as sparse support.