fix(encoding): plan sparse structural miniblock pages - #6787
Conversation
|
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@claude review once |
There was a problem hiding this comment.
Reviewed: no bugs found, but this is a substantial change to structural page planning and rep/def serialization (a format-critical code path) — worth a careful human pass, especially the new split-planning state machine in repdef.rs and the unsplittable-over-budget fallback logic in primitive.rs.
Extended reasoning...
Overview
This PR moves the sparse-structural-page fix from a value-layer fullzip fallback into the rep/def serialization layer. It introduces a StructuralPagePlan (Fits / Split / UnsplittableOverBudget) computed during level normalization, then the primitive encoder either encodes the page as-is, splits on top-level row boundaries, or falls back to fullzip when a single row is too large for one mini-block chunk. It also factors the per-page encode path into a PrimitiveEncodeContext + encode_page pair and replaces several assert! / panic! paths with proper Error::invalid_input_source returns. Touches 4 files in lance-encoding, all in the on-disk encoding pipeline.
Security risks
No external input is parsed; no auth/crypto/permission surface. The replaced assert!/panic! sites are encoder-internal and now return errors instead, which is a small robustness improvement. Risk surface is correctness of the on-disk format, not security.
Level of scrutiny
High. This is format-critical code: rep/def serialization is central to how Lance writes nested data, and the new state machine in normalize_specials_and_plan_splits interleaves special-value normalization with row-boundary tracking and budget accounting. Off-by-one mistakes in finish_row, level_range, or value_start would silently produce subtly-wrong pages. The UnsplittableOverBudget branch in encode_page also has a fairly large match arm enumerating which value-block shapes fullzip can/can't accept — easy to get a case wrong. New test coverage is reasonable (sparse boolean, long empty/null prefixes, nested fullzip fallback, mini-block error path) but patch coverage is 81% with 144 uncovered lines, much of it in the new fallback/error branches.
Other factors
The bug-hunting system did not flag anything. The replacement of assert_eq!/panic! with error returns in serialize_full_zip_fixed / serialize_full_zip_variable is a behavioral change (panic → error) — benign but worth a reviewer noting. PR author requested @claude review once explicitly, and the diff replaces a prior in-tree approach (repdef_too_sparse_for_miniblock), so a human should confirm the new approach is the agreed direction rather than a competing implementation.
westonpace
left a comment
There was a problem hiding this comment.
This is a significant change to the write path and write corruption is always a fear so I might do some additional manual testing of this fix.
That being said, the change seems to be very narrowly enacted. We only use this on sparse lists.
We are moving repdef serialization out of the spawned task so this means that work will be serialized. This may have some detrimental impacts to write speed. It would be good to run some write benchmarks just to understand what these are.
Performance-wise I do worry this could lead to runt pages. We want to have MBs of data per page. This helps avoid overly small reads when hitting object storage. Since this planning is splitting pages at 2^16 values I think we will get rather tiny pages.
Perhaps in 2.3 we can go ahead and add a "midi-block encoding" where we don't try and fit all the chunk metadata into 2 bytes. That would also help address other cases where users are asking for much larger compression windows.
Anyways, I'm sitting at a soft +1 for now. I'll do some additional write testing and try and run some writer benchmarks just to see if there is any impact and then change this to approve.
|
I've filed #6841 to follow-up on a longer term fix. |
I ran through some write benchmarks and didn't see any noticeable difference. I think in most typical cases the repdef time is very small so it doesn't matter. |
Resolve conflicts with #6989: drop the competing repdef_too_sparse_for_miniblock / any_chunk_levels_overflow_u16 heuristic in favor of this PR's structural page splitting, which keeps the dense prefix on mini-block pages instead of falling back to fullzip. Port #6989's HNSW regression tests with assertions updated to expect the split mini-block layout.
westonpace
left a comment
There was a problem hiding this comment.
Forgot to check back in here. The write benchmarks didn't show any noticeable impact. Let's move forwards
…ack (#8934) When a single top-level row carries more rep/def levels than one mini-block chunk can hold, the primitive encoder falls back to full-zip after a pre-check that the value block is something full-zip can serialize. That pre-check rejected 1-bit booleans as non-byte-aligned, although `encode_full_zip` widens them to bytes before compressing (#6723). A sparse `List<List<Boolean>>` row therefore failed with "Mini-block cannot encode N rep/def levels in one top-level row" even though it encodes fine, and even when the user explicitly requested `structural_encoding=fullzip`. Before #6787 the same row was written through full-zip. The pre-check now lets 1-bit fixed-width blocks through, matching what `encode_full_zip` accepts. The boolean test that asserted the error now asserts a full-zip round trip alongside the existing string case.
Root cause
Mini-block page planning was effectively bounded by visible leaf values, but sparse nested lists can contain many rep/def structural events with few or no visible values. Those pages can fit the value budget while still overflowing mini-block's packed rep/def chunk metadata. Falling back to fullzip at the primitive value layer was the wrong level of fix because fullzip only applies to value layouts it can actually encode and should not become the generic escape hatch for structural streams.
Fix
This moves the decision to the structural stream. During rep/def serialization, Lance now produces a structural page plan while normalizing levels. The primitive encoder consumes that plan and splits oversized structural pages on top-level row boundaries before mini-block encoding. Splits with no visible leaf values use the existing complex all-null layout, value-bearing splits continue through mini-block, and a single row that is itself too large falls back to fullzip only when the value layout is compatible; otherwise it returns a clear error.
This keeps the on-disk format compatible: it emits only existing page layouts and does not add a new layout variant or metadata field. It also preserves the format's random-access model by making structural pages independently addressable instead of treating sparse nested structure as a value-encoding special case.
Validation
Targeted sparse nested list regressions and the full lance-encoding test suite pass. Microbenchmarks show no meaningful dense-path regression; the sparse boolean encode regression from the previous intermediate-summary approach is gone, and nested sparse string encoding remains faster.