Skip to content

fix: handle nullable validity layers without def levels#6187

Merged
Xuanwo merged 2 commits intomainfrom
xuanwo/fix-6185-repdef-validity
Mar 13, 2026
Merged

fix: handle nullable validity layers without def levels#6187
Xuanwo merged 2 commits intomainfrom
xuanwo/fix-6185-repdef-validity

Conversation

@Xuanwo
Copy link
Copy Markdown
Collaborator

@Xuanwo Xuanwo commented Mar 13, 2026

This fixes the reader panic in #6185 when a page keeps nullable rep/def layer metadata but does not materialize any definition levels. The decoder now treats that page-local state as all-valid and includes a regression test that reproduces the mixed-page case before the fix.

Closes #6185.

@github-actions github-actions Bot added the bug Something isn't working label Mar 13, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Review

Clean, well-scoped fix for the panic in #6185. The approach of treating def_levels: None as all-valid is correct — if no definition buffer was materialized, there are no nulls to decode.

One minor observation:

In skip_validity and the early-return path of unravel_validity, the fix advances current_def_cmp += meaning.num_def_levels(). When def_levels is None, this counter is effectively dead (all subsequent layers will also hit the def_levels.is_none() bail-out), so it's harmless — but it's also good practice to keep the state consistent in case the invariants change later. Looks fine as-is.

The regression test correctly covers the mixed-page scenario (one page with def levels, one without). LGTM.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@Xuanwo Xuanwo merged commit 0851b14 into main Mar 13, 2026
29 checks passed
@Xuanwo Xuanwo deleted the xuanwo/fix-6185-repdef-validity branch March 13, 2026 07:28
westonpace pushed a commit that referenced this pull request Mar 17, 2026
This fixes the reader panic in #6185 when a page keeps nullable rep/def
layer metadata but does not materialize any definition levels. The
decoder now treats that page-local state as all-valid and includes a
regression test that reproduces the mixed-page case before the fix.

Closes #6185.
westonpace added a commit that referenced this pull request Mar 18, 2026
## Summary

Cherry-picks bug fixes onto `release/v3.0` for the v3.0.1 patch release:

- **#6160** - fix: handle `DataType::Null` in `adjust_child_validity` to
prevent panic
- **#6187** - fix: handle nullable validity layers without def levels
- **#6143** - fix: prevent duplicate manifest entries from concurrent
table creation
- **#6212** - chore: bump lz4_flex patch versions
- **#6146** - fix: replace fetch_arrow_table with to_arrow_table

## Test plan

- CI passes on cherry-picked commits (both PRs were already merged and
tested on main)

---------

Co-authored-by: Will Jones <willjones127@gmail.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Xuanwo <github@xuanwo.io>
Co-authored-by: Jonathan Hsieh <jon@lancedb.com>
Co-authored-by: BubbleCal <bubble-cal@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

panic: unwrap on None in RepDefUnraveler::unravel_validity when def_levels is absent but def_meaning is non-trivial

2 participants