Skip to content

fix(encoding): derive full-zip max_visible_def like the writer - #9254

Open
westonpace wants to merge 1 commit into
lance-format:mainfrom
westonpace:fix/full-zip-invisible-items
Open

westonpace wants to merge 1 commit into
lance-format:mainfrom
westonpace:fix/full-zip-invisible-items

Conversation

@westonpace

Copy link
Copy Markdown
Member

The full-zip decoder summed the definition levels of every non-list layer, including layers outside the first list, while the writer counts only the layers beneath it. When a nullable layer sits above a list -- for example struct<x: list> with both the struct and the list nullable -- the decoder's threshold lands above the level written for a null list, so it hands a leaf value to items that were written as invisible and every later value shifts by one. Depending on the leaf type this silently corrupts data, trips the structural validity check, or panics while slicing the page buffer.

The bytes on disk are correct, so this is a read-path fix: existing files decode correctly once the reader agrees with the writer, with no rewrite.

Three other decoders (mini-block, complex-all-null and constant) already had the rule right. Move it into a single shared max_visible_level in repdef and call it from the writer and all four decoders so the two sides cannot drift again.

Also bounds check the fixed full-zip item walk. It is driven by the page's own control words, so a malformed page could read a truncated control word, advance past the end of the buffer, or exhaust the buffer queue -- each of which aborted the process instead of returning a corrupt-file error.

The full-zip decoder summed the definition levels of every non-list layer,
including layers outside the first list, while the writer counts only the
layers beneath it. When a nullable layer sits above a list -- for example
struct<x: list<int32>> with both the struct and the list nullable -- the
decoder's threshold lands above the level written for a null list, so it
hands a leaf value to items that were written as invisible and every later
value shifts by one. Depending on the leaf type this silently corrupts data,
trips the structural validity check, or panics while slicing the page buffer.

The bytes on disk are correct, so this is a read-path fix: existing files
decode correctly once the reader agrees with the writer, with no rewrite.

Three other decoders (mini-block, complex-all-null and constant) already had
the rule right. Move it into a single shared `max_visible_level` in repdef and
call it from the writer and all four decoders so the two sides cannot drift
again.

Also bounds check the fixed full-zip item walk. It is driven by the page's own
control words, so a malformed page could read a truncated control word, advance
past the end of the buffer, or exhaust the buffer queue -- each of which
aborted the process instead of returning a corrupt-file error.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@westonpace
westonpace requested a review from Xuanwo September 15, 2026 21:43
@github-actions github-actions Bot added A-encoding Encoding, IO, file reader/writer bug Something isn't working labels Sep 15, 2026

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: approve.

The reader now applies the writer’s leaf-first visibility boundary, which restores existing full-zip pages with nullable ancestors above a list without changing on-disk bytes. Sharing that derivation across the writer and dense decoders prevents the contract from drifting again, and the regression coverage exercises both fixed- and variable-width leaves. The fixed-width bounds checks also convert truncated page walks into corrupt-file errors.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-encoding Encoding, IO, file reader/writer bug Something isn't working K-approved Latest Gatekeeper recommendation permits acceptance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant