Repository navigation
Change extension dtype nullability without validating again - #11012
connortsui20 wants to merge 2 commits into
Conversation
Merging this PR will regress 1 benchmark2 warnings
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing |
963b7ec to
35c68ec
Compare
|
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 <connor.tsui20@gmail.com>
35c68ec to
f2c669e
Compare
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
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.