Repository navigation
Reject nullable nested levels in list-based spatial types - #11183
Merged
Merged
Conversation
GeoArrow allows nulls only at the outer level of a geometry, and the storage helpers build every inner List and the coordinate Struct as non-nullable. The validators of LineString, MultiPoint, Polygon, MultiLineString, and MultiPolygon only checked the coordinate fields, so they also accepted a nullable inner level. Each validator now rebuilds the canonical storage at the stored top-level nullability and requires equality. No Vortex producer wrote such a dtype, but a file built by hand through the Rust API with one now fails to read. Signed-off-by: Connor Tsui <[email protected]>
Merging this PR will degrade performance by 11.06%1 warning
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
robert3005
approved these changes
Nov 4, 2026
connortsui20
added a commit
that referenced
this pull request
Nov 4, 2026
TS (#10953) merged on 2026-11-03, and SN1 (#11183) and V10 (#11187) merged on 2026-11-04. The dependency graph marks the three merged and moves its state date to 2026-11-04. It gives SD, DP, FD and U9 their open PRs (#11184, #11185, #11186 and #11188), and it drops #10408, which closed without a merge. The text of parts 4 and 5 and the Fix lines of EXT-V9, EXT-V10, EXT-N2 and EXT-S3 now record the same state. Also record that AX, DS and SN2 are implemented on branches without a PR, and add the approved one-pass design for the masked nulls of SN2. Part 2 and decision X31 also record the fixes: #10953 made every i64 legal, #11183 made the spatial validators reject nullable nested levels, #11187 gave the Date display its fallback, and #10408 closed. Signed-off-by: Connor Tsui <[email protected]>
connortsui20
added a commit
that referenced
this pull request
Nov 13, 2026
## Summary - Tracking Issue: #11114 Right now, an extension type validates whatever storage dtype the caller hands it, so one type can have several storage forms, and its metadata doesn't say which one a dtype uses. Each `ExtVTable` now declares its storage from its metadata, so the metadata **deterministically fixes / decides** the whole storage dtype except the top-level nullability. Reads then require the stored storage to equal that declaration exactly, **including nested nullability**. This has to be strict, since arrays decode their storage child with the declared dtype, so a stored form that differs would be misread instead of rejected. Every built-in type accepts the same stored forms as before, so existing files still read. _NB on nested nullability: this means that if we potentially need nulls in the storage, an extension type MUST require its storage type to be nullable, even if in some cases it might not be. I think this is a worthwhile tradeoff for clarity of implementation._ ## Changes Some facts lived only in the storage dtype, like the element type of `Vector` and `FixedShapeTensor` and the dimension of the native spatial types, so they move into the in-memory `Metadata`. `deserialize_metadata` decodes `StoredMetadata` (which is the old `Metadata`, and `metadata_from_storage` combines it with storage to rebuild complete metadata. The serialized bytes don't change. Some other things: - File reads and storage-based dtype, array, and scalar constructors share the same checked path. - `ForeignExtDType` can't derive its storage, since it doesn't know its type, so it keeps the stored storage in its metadata and declares that. - The nested-nullability checks that #11183 added to the spatial helpers go away, since the read check now covers them. Follow-up: a read error in a struct field still panics on first access (SF in #11114). ## API Changes - `ExtVTable::validate_dtype` becomes `storage_dtype` (plus an optional `validate_metadata`), `deserialize_metadata` returns `StoredMetadata`, and `metadata_from_storage` reconstructs complete metadata from stored metadata and storage. - `ExtDType::try_new` and `try_with_vtable` take a nullability in place of the storage dtype. - Reading an extension dtype whose stored storage differs from its declaration fails, and so does building a scalar or array over the wrong storage. - `FixedShapeTensorMetadata::new` takes the element type, `Vector`'s metadata is `VectorMetadata`, metadata-first construction of native spatial dtypes needs `SpatialMetadata::with_dimension`, and `SpatialMetadata` no longer implements `prost::Message`. - `ExtDType` and `ExtensionArray` gain `try_from_storage` constructors, and `Scalar` gains `try_extension_from_storage`, with explicit-vtable variants. Existing metadata-first constructors remain available. - `FixedShapeTensorStoredMetadata` supplies shape and layout, and `SpatialStoredMetadata` supplies CRS when constructing from storage. - Fallible constructors reject non-nullable null storage, including extensions over null storage, with an error. --------- Signed-off-by: Connor Tsui <[email protected]>
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
GeoArrow only allows nulls at the outer level of a geometry, but the validators of the list-based spatial types only checked the coordinate fields, so
LineStringalso acceptedlist({x=f64, y=f64}?).LineString,MultiPoint,Polygon,MultiLineString, andMultiPolygonnow accept only the storage that their helper builds, at either top-level nullability. That leaves each type one storage form for each dimension, which the storage declarations (SD in #11114) need.Changes
Each validator compares the stored dtype with its storage helper, so the helpers stay the only definition of the form.
PointandRectalready rejected nullable coordinate fields, and WKB has no nested level. Tests cover every nested level of each type, and reading a serializedvortex.st.linestringwhose coordinateStructis nullable.API Changes
Building one of these five types with a nullable inner
Listor coordinateStruct(e.g., withExtDType::try_new) now returns an error, and so does reading a dtype in that form. No Vortex writer produces that form, so only storage built by hand through the Rust API can hit it.Note that a file keeps its columns as fields of a struct whose field dtypes are parsed lazily, so reading such a column from a file currently panics instead of returning the error. A follow-up fixes that.