refactor(encoding): clarify mini-block repdef budget by Xuanwo · Pull Request #7751 · lance-format/lance

Xuanwo · 2026-07-13T10:35:25Z

Part of #7750

This isolates and names the existing dense mini-block rep/def page-budget decision so later sparse auto-selection can consume an honest contract. It renames the planner result and related encoder methods and fields without changing the underlying decision or page splitting.

There is no format or behavior change.

Validation:

cargo fmt --all
cargo test -p lance-encoding (427 passed, 5 ignored; doc-tests: 0 passed, 1 ignored)
cargo clippy -p lance-encoding --tests --benches -- -D warnings
cargo clippy --all --tests --benches -- -D warnings

Summary by CodeRabbit

Bug Fixes
- Improved handling of pages with large repetition and definition-level data.
- Prevented encoding failures when a single row exceeds mini-block limits.
- Added safer fallback behavior for oversized rows during page encoding.
- Improved page splitting to keep data within encoding budgets while preserving correctness.

coderabbitai · 2026-07-13T10:35:31Z

📝 Walkthrough

Walkthrough

Rep/def serialization now returns a mini-block budget result. Primitive page splitting and encoding use that result to handle within-budget pages, split pages, and single-row over-budget pages.

Changes

Mini-block rep/def budgeting

Layer / File(s)	Summary
Budget result types and split planning `rust/lance-encoding/src/repdef.rs`	Adds `MiniBlockRepDefSplit` and `MiniBlockRepDefBudget`, and updates level scanning to classify pages as within budget, splittable, or single-row over budget.
Budgeted rep/def serialization `rust/lance-encoding/src/repdef.rs`	Updates builder and serialization entrypoints to return serialized rep/def data with the new budget result.
Primitive page splitting and encoding `rust/lance-encoding/src/encodings/logical/primitive.rs`	Replaces structural page splitting with budget-based splitting and updates page metadata, flush wiring, and mini-block encoding checks.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PrimitiveFlush
  participant RepDefBuilder
  participant BudgetPlanner
  participant PageSplitter
  participant encode_page
  PrimitiveFlush->>RepDefBuilder: serialize_with_miniblock_repdef_budget
  RepDefBuilder->>BudgetPlanner: normalize_specials_and_plan_splits
  BudgetPlanner-->>RepDefBuilder: MiniBlockRepDefBudget
  RepDefBuilder-->>PrimitiveFlush: SerializedRepDefs and budget
  PrimitiveFlush->>PageSplitter: split_pages_for_miniblock_repdef_budget
  PageSplitter-->>PrimitiveFlush: budgeted primitive pages
  PrimitiveFlush->>encode_page: encode page with single-row budget marker

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately summarizes the refactor around mini-block rep/def budget planning and encoding.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch xuanwo/sparse-stack-1-repdef-budget

_{Comment @coderabbitai help to get the list of available commands.}

claude

Claude Code Review

Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.

_{Tip: disable this comment in your organization's Code Review settings.}

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

rust/lance-encoding/src/repdef.rs (1)
803-809: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Function name is now stale relative to its new MiniBlockRepDefBudget contract.

normalize_specials_and_plan_splits now decides between WithinBudget, RequiresPageSplit, and SingleRowOverBudget — not just "plan splits". Given the PR's explicit goal of making the budget decision explicit for future sparse auto-selection, renaming this helper (e.g., normalize_specials_and_plan_miniblock_repdef_budget) would keep the internal naming consistent with the new public contract.

As per coding guidelines: "Name functions to match their actual scope; for example, handle_partition_system_columns instead of handle_system_columns if only a subset is handled."
🤖 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/repdef.rs` around lines 803 - 809, Rename
normalize_specials_and_plan_splits to reflect that it normalizes specials and
determines the MiniBlockRepDefBudget outcome, such as
normalize_specials_and_plan_miniblock_repdef_budget. Update every call site and
any related references while preserving its existing behavior and return
contract.
Source: Coding guidelines
rust/lance-encoding/src/encodings/logical/primitive.rs (1)
3790-3801: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document the absent-state meaning and value semantics of single_row_miniblock_repdef_levels.

The comment only explains when the field is Some; it doesn't state what the wrapped u64 represents (number of rep/def levels for the offending row) nor what None implies (row fits within the mini-block budget). The paired enum doc in repdef.rs (SingleRowOverBudget) already spells this out — mirroring it here would help.

As per coding guidelines: "Document the semantic meaning of both present and absent states for Option<T> fields."
📝 Suggested doc tweak
-    // Present when one top-level row is too large for one mini-block rep/def page.
+    // `Some(num_levels)` when one top-level row alone has `num_levels` rep/def levels that
+    // exceed the mini-block budget (splitting cannot help). `None` means the page's rep/def
+    // stream fits the mini-block budget as-is or after row-boundary splitting.
     single_row_miniblock_repdef_levels: Option<u64>,
🤖 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 3790 -
3801, Update the documentation for
PrimitivePageData::single_row_miniblock_repdef_levels to describe both Option
states: Some(u64) contains the number of repetition/definition levels for the
offending oversized row, while None means the row fits within the mini-block
budget. Mirror the semantic wording used by SingleRowOverBudget in repdef.rs.
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.

Outside diff comments:
In `@rust/lance-encoding/src/encodings/logical/primitive.rs`:
- Around line 3790-3801: Update the documentation for
PrimitivePageData::single_row_miniblock_repdef_levels to describe both Option
states: Some(u64) contains the number of repetition/definition levels for the
offending oversized row, while None means the row fits within the mini-block
budget. Mirror the semantic wording used by SingleRowOverBudget in repdef.rs.

In `@rust/lance-encoding/src/repdef.rs`:
- Around line 803-809: Rename normalize_specials_and_plan_splits to reflect that
it normalizes specials and determines the MiniBlockRepDefBudget outcome, such as
normalize_specials_and_plan_miniblock_repdef_budget. Update every call site and
any related references while preserving its existing behavior and return
contract.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b2bfdc1f-a283-43f9-9e92-e1c4451b3310

📥 Commits

Reviewing files that changed from the base of the PR and between e7bce1e and 7bcb4b0.

📒 Files selected for processing (2)

rust/lance-encoding/src/encodings/logical/primitive.rs
rust/lance-encoding/src/repdef.rs

codecov · 2026-07-13T11:19:53Z

Codecov Report

❌ Patch coverage is 95.12195% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
.../lance-encoding/src/encodings/logical/primitive.rs	94.44%	1 Missing ⚠️
rust/lance-encoding/src/repdef.rs	95.65%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Part of #7750 ## Summary - In Lance 2.3+, automatically choose sparse structural pages only when the structural encoding is implicit, the dense mini-block rep/def plan requires top-level page splitting or has a single row over budget, and the sparse writer supports the value path. - Keep within-budget pages on the dense path without constructing a sparse value block or sparse plan. - Preserve every explicit structural mode, Lance 2.2 behavior, and dense fallback/error behavior for dictionary, variable packed-struct, and other sparse-ineligible value paths. - Keep the policy decision separate from sparse serialization; this adds no wire-format fields. 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 - [PR3 #7754](#7754) absorbed the readable and explicitly writable sparse format from the closed [PR4 #7755](#7755). Its squash patch is replayed directly on the current `main` base before the policy commits in this PR. - [PR1 #7751](#7751) is merged; the rebase resolves the stack onto its `MiniBlockRepDefBudget` naming. ## 100M-row S3 benchmark Environment: `r7i.8xlarge` in `us-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 at `66c936391aa46bfcd14f7ac1c820878da68fcc0b` on `xuanwo/sparse-stack-5-auto-sparse-bench-final`, based on pre-rebase PR5 head `ed0ce68d7f4cceafe6a249c912f7f1da8c4fecd9`. 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. `cold` is the first operation on a newly opened `Dataset`; `warm` is the immediate repeat on the same `Dataset`. Times are milliseconds. Bytes are total dataset object bytes. Every dataset has 102 objects, including 100 Lance data files. | Case | Mode | Bytes | Pages | Cold full scan | Cold random | Warm full scan | Warm random | Warm empty | Warm non-empty | | --- | --- | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | | hnsw | sparse | 1,600,850,504 | 317 | 1,627.830 | 466.940 | 1,472.636 | 272.931 | 0.484 | 29.238 | | hnsw | mini-block | 1,663,361,256 | 328 | 1,720.362 | 484.619 | 1,655.289 | 273.812 | 0.466 | 20.664 | | hnsw | full-zip | 2,342,961,549 | 328 | 3,239.070 | 478.839 | 3,104.055 | 273.603 | 0.659 | 25.735 | | uniform | sparse | 117,091 | 100 | 99.990 | 77.346 | 76.518 | 3.408 | 0.548 | 28.299 | | uniform | mini-block | 689,601 | 1,600 | 111.844 | 137.503 | 606.852 | 123.301 | 93.404 | 94.822 | | uniform | full-zip | 496,906,995 | 1,600 | 1,589.645 | 1,205.110 | 650.542 | 359.496 | 72.292 | 58.997 | | deep | sparse | 1,144,900 | 300 | 159.361 | 77.122 | 135.065 | 9.309 | 0.884 | 25.580 | | deep | mini-block | 376,199,150 | 10,100 | 6,735.485 | 4,673.904 | 2,560.397 | 582.959 | 174.014 | 189.929 | | deep | full-zip | 994,111,217 | 10,100 | failed | failed | failed | failed | failed | failed | `deep/full-zip` wrote all 100M rows and produced the reported size/page/object counts, then its first full scan panicked at `primitive.rs:2588` by 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 SHA `da06b79a7f5eda4f33e2d80990c46766095ba383` independently confirmed that this nested-null `deep/full-zip` read was already broken before this PR, returning `Structural 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.

refactor(encoding): clarify mini-block repdef budget

7bcb4b0

github-actions Bot added the A-encoding Encoding, IO, file reader/writer label Jul 13, 2026

Xuanwo marked this pull request as ready for review July 13, 2026 10:36

claude Bot reviewed Jul 13, 2026

View reviewed changes

BubbleCal approved these changes Jul 13, 2026

View reviewed changes

coderabbitai Bot reviewed Jul 13, 2026

View reviewed changes

Xuanwo merged commit d6fb34d into main Jul 13, 2026
34 checks passed

Xuanwo deleted the xuanwo/sparse-stack-1-repdef-budget branch July 13, 2026 10:55

Xuanwo mentioned this pull request Jul 13, 2026

perf(encoding): auto-select sparse structural pages #7756

Merged

Xuanwo mentioned this pull request Jul 22, 2026

Tracking: Land native sparse structural layout in Lance 2.3 #7750

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(encoding): clarify mini-block repdef budget#7751

refactor(encoding): clarify mini-block repdef budget#7751
Xuanwo merged 1 commit into
mainfrom
xuanwo/sparse-stack-1-repdef-budget

Xuanwo commented Jul 13, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 13, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

claude Bot left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

codecov Bot commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Xuanwo commented Jul 13, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Jul 13, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Xuanwo commented Jul 13, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 13, 2026 •

edited

Loading