fix(encoding): derive full-zip max_visible_def like the writer - #9254
Open
westonpace wants to merge 1 commit into
Open
westonpace wants to merge 1 commit into
westonpace wants to merge 1 commit into
Conversation
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>
Contributor
There was a problem hiding this comment.
✅ 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_levelin 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.