Repository navigation
Change extension dtype nullability without validating again - #11012
Merged
Merged
Conversation
connortsui20
added
the
changelog/break
ChatGPT Codex Connector
Merging this PR will degrade performance by 13.98%2 warnings
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
|
connortsui20
marked this pull request as draft
October 30, 2026 00:13
connortsui20
force-pushed
the
ct/ext-types/dtype-invariants
branch
2 times, most recently
from
October 30, 2026 13:46
963b7ec to
35c68ec
Compare
Member
Author
|
So this is definitely wrong, and it seems like we need to do a bit more design work around storage dtypes and nullability |
An extension type cannot restrict whether its values can be null, so `ExtVTable::validate_dtype` must not depend on the top-level nullability of the storage dtype. `ExtDType::with_nullability` now relies on that contract and rebuilds the dtype without validating again, so nullability changes in `take`, `mask` and `min_max` cannot panic. Also fixes `Debug for ExtDTypeRef`, which printed the metadata before the struct and then `metadata: Ok(())`. Signed-off-by: Connor Tsui <[email protected]>
connortsui20
changed the title
connortsui20
force-pushed
the
ct/ext-types/dtype-invariants
branch
from
October 30, 2026 19:34
35c68ec to
f2c669e
Compare
connortsui20
marked this pull request as ready for review
October 30, 2026 21:10
Signed-off-by: Connor Tsui <[email protected]>
robert3005
approved these changes
Nov 2, 2026
connortsui20
enabled auto-merge (squash)
November 2, 2026 13:50
Signed-off-by: Connor Tsui <[email protected]>
9 tasks done
connortsui20
added a commit
that referenced
this pull request
Nov 2, 2026
U2, U3, U4, U6, U7 and U8 merged on 2026-11-02 in #11012, #11014 and #11016, and the DataFusion literal fix #11067 merged right after U2. So order constraint 1 holds, and phase 1 starts from develop. The graph shows these nodes as merged, and parts 2 and 4 name the PR that fixed each finding. #10953 now keeps i32 days and widens only the days part to i64 when a value does not fit. It has no review, and it conflicts with the DateTimeParts compare kernel that U2 changed. #11056 still waits on its request for changes. #10661 replaces the closed #10482 for file sort statistics. In the type roadmap, only the N1 sentence changes. Signed-off-by: Connor Tsui <[email protected]>
12 of 49 tasks
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.
Summary
takeandmaskcan make an extension array nullable, whilemin_maxbuilds non-null result dtypes. These operations need to change nullability without failing. Previously,ExtDType::with_nullabilitycalled the plugin validator again and panicked if it returned an error.An extension type must accept either top-level nullability, so an already valid dtype can be reused with that flag changed.
with_nullabilitynow relies on this contract instead of validating again.Changes
Keeps metadata and storage types intact when changing nullability. Also fixes erased dtype
Debugoutput, which wrote metadata before the struct and displayedmetadata: Ok(())in its place.Regression tests cover both nullabilities, validation call counts, and compact and pretty
Debugoutput. Checks for the style cleanup are left to CI.API Changes
Documents that
ExtVTable::validate_dtypemust be independent of the storage dtype's top-level nullability. Validators can still constrain nullability inside nested storage types.