Skip to content

Reject nullable nested levels in list-based spatial types - #11183

Merged
connortsui20 merged 1 commit into
developfrom
ct/spatial-nested-nonnull
Nov 4, 2026
Merged

connortsui20 merged 1 commit into
developfrom
ct/spatial-nested-nonnull

Conversation

@connortsui20

Copy link
Copy Markdown
Member

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 LineString also accepted list({x=f64, y=f64}?). LineString, MultiPoint, Polygon, MultiLineString, and MultiPolygon now 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. Point and Rect already rejected nullable coordinate fields, and WKB has no nested level. Tests cover every nested level of each type, and reading a serialized vortex.st.linestring whose coordinate Struct is nullable.

API Changes

Building one of these five types with a nullable inner List or coordinate Struct (e.g., with ExtDType::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.

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]>
@connortsui20 connortsui20 added the changelog/break A breaking API change label Nov 3, 2026
@connortsui20
connortsui20 marked this pull request as ready for review November 3, 2026 22:51
@connortsui20 connortsui20 mentioned this pull request Nov 3, 2026
12 of 49 tasks
@codspeed

codspeed Bot commented Nov 3, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 11.06%

1 warning

⚠️ Walltime results may be unreliable

Some benchmarks used the Walltime instrument outside CodSpeed Macro Runners, which can make the results inconsistent.

Learn more about Macro Runners

See affected benchmarks

❌ 1 regressed benchmark
✅ 1764 untouched benchmarks
⏩ 611 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Walltime lt_i64_nullable_avx2 8.4 µs 9.5 µs -11.06%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ct/spatial-nested-nonnull (ea28937) with develop (bc58587)

Open in CodSpeed

Footnotes

  1. 611 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@connortsui20
connortsui20 merged commit 956c1ea into develop Nov 4, 2026
171 of 173 checks passed
@connortsui20
connortsui20 deleted the ct/spatial-nested-nonnull branch November 4, 2026 12:26
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/break A breaking API change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants