perf(encoding): auto-select sparse structural pages#7756
Conversation
📝 WalkthroughWalkthroughAdds Lance 2.3+ sparse structural page writing with automatic selection, prepared value serialization, value-path validation, rep/def plan borrowing, tests, and format documentation. ChangesSparse structural page layout
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 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! |
westonpace
left a comment
There was a problem hiding this comment.
These rules make sense to me
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance-encoding/src/encodings/logical/primitive.rs (1)
5754-5765: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse one sparse eligibility contract for every selection path.
Automatic selection, explicit sparse requests, and value preparation currently validate different subsets of
DataBlockvariants. Centralize the recursive predicate and invoke it for explicit requests and insideprepare_values.
rust/lance-encoding/src/encodings/logical/primitive.rs#L5754-L5765: validate explicit sparse values before constructing the sparse page.rust/lance-encoding/src/encodings/logical/primitive/sparse/writer.rs#L359-L363: recurse through struct children instead of checking only variable-width descendants.rust/lance-encoding/src/encodings/logical/primitive/sparse/writer.rs#L539-L562: enforce the same predicate and return caller-facing errors for rejected inputs.🤖 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.rs` around lines 5754 - 5765, Unify sparse eligibility around the shared predicate used by sparse planning: in rust/lance-encoding/src/encodings/logical/primitive.rs#L5754-L5765, validate explicit sparse values before constructing the sparse page; in rust/lance-encoding/src/encodings/logical/primitive/sparse/writer.rs#L359-L363, update the predicate to recurse through struct children; and in rust/lance-encoding/src/encodings/logical/primitive/sparse/writer.rs#L539-L562, invoke that predicate during value preparation and return caller-facing errors for rejected inputs.
🧹 Nitpick comments (1)
rust/lance-encoding/src/encodings/logical/primitive.rs (1)
5781-5803: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBuild the structural plan before compressing candidate values.
This closure fully prepares and serializes values before checking whether the plan uses constant layout. That work is discarded when sparse is rejected, and dense encoding may compress the same values again. Move plan construction and the constant-layout gate before
prepare_values.🤖 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.rs` around lines 5781 - 5803, In the sparse encoding closure, move sparse::writer::plan(&normalized, num_values)? and the sparse::writer::uses_constant_layout(&plan, &self.field) early-return before constructing DataBlock or calling sparse::writer::prepare_values. Keep the supports_value_block check and preparation/error handling after this gate so candidate values are only compressed when the structural plan is eligible.
🤖 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.
Outside diff comments:
In `@rust/lance-encoding/src/encodings/logical/primitive.rs`:
- Around line 5754-5765: Unify sparse eligibility around the shared predicate
used by sparse planning: in
rust/lance-encoding/src/encodings/logical/primitive.rs#L5754-L5765, validate
explicit sparse values before constructing the sparse page; in
rust/lance-encoding/src/encodings/logical/primitive/sparse/writer.rs#L359-L363,
update the predicate to recurse through struct children; and in
rust/lance-encoding/src/encodings/logical/primitive/sparse/writer.rs#L539-L562,
invoke that predicate during value preparation and return caller-facing errors
for rejected inputs.
---
Nitpick comments:
In `@rust/lance-encoding/src/encodings/logical/primitive.rs`:
- Around line 5781-5803: In the sparse encoding closure, move
sparse::writer::plan(&normalized, num_values)? and the
sparse::writer::uses_constant_layout(&plan, &self.field) early-return before
constructing DataBlock or calling sparse::writer::prepare_values. Keep the
supports_value_block check and preparation/error handling after this gate so
candidate values are only compressed when the structural plan is eligible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 4f94d639-da08-414d-a8ce-4f466070132d
📒 Files selected for processing (3)
rust/lance-encoding/src/encodings/logical/primitive.rsrust/lance-encoding/src/encodings/logical/primitive/sparse/writer.rsrust/lance-encoding/src/repdef.rs
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.