fix(table): normalize EWKB geometry values on write - #1670
Conversation
zeroshade
left a comment
There was a problem hiding this comment.
The core conversion logic in normalizeWKB (table/internal/geo_codec.go) is correct — byte order preserved, ISO passthrough zero-copy, SRID stripped — and the test coverage (sliced containers, null masking at every nesting level, end-to-end Parquet round-trip) is genuinely thorough. Tests build and pass on the PR head. A few things I'd like addressed/answered before this leaves draft:
-
Per-batch overhead for non-geo schemas.
normalizeGeoBatchrecurses into every column of every batch vianormalizeNestedArrayReachable(table/internal/parquet_files.go). For any nested column — geo or not —normalizeListChildallocates achildActive []boolsized to the full child value range, slices the values array, and walks all offsets, on everyWritecall. For a writer streaming large batches of nested non-geo data this is measurable pure waste. A cheap datatype-level pre-check (does the column'sarrow.DataTypecontain a*geoarrow.WKBTypeanywhere — computable once per writer, or at least once per column type) would make the common no-geo case zero-cost. SinceparquetFormat.NewFileWriteralready inspects the schema for WKB types around line 404, the writer could cache which column indices even need visiting. -
Undocumented behavior change on the read/bounds path.
decodeWKBnow passesEmptyPointHandlingNaNtowkb.Unmarshal, so ISO WKB empty points (NaN coords) that previously failed bounds accumulation — and therefore failed the write — are now accepted. That looks like a deliberate and reasonable fix (the accumulator already documents NaN-skipping), but it's a distinct behavior change not mentioned in the PR description. Please call it out there, or split it if it's separable. Also note the asymmetry:normalizeWKB'sewkb.Unmarshal(data)call doesn't pass the empty-point option while the marshal side does — the empty-point tests pass, so it works today, but a one-line comment on why the option is only needed on the ISO side would prevent someone "fixing" the asymmetry later. -
Confirming intent on masked values: values hidden behind null ancestors are copied verbatim (still EWKB, and malformed bytes don't error), while the same bytes in a reachable slot would either be normalized or fail the write. I see from the tests this is deliberate — it avoids failing writes on garbage in masked slots — but it does mean written files can still contain EWKB bytes in unreachable child positions. Fine by me, just want it confirmed as the intended contract since
accumulateGeoBoundsmakes the same reachability distinction.
Minor: dictionary- or REE-encoded columns wrapping a WKB extension type would silently pass through unnormalized; probably impossible via the current write path, but worth a defensive comment if so.
Since this is a draft I'm leaving a comment rather than a formal review outcome.
Summary
Normalize EWKB geometry values to ISO WKB before writing Parquet data.
Why
EWKB values could be read and have their bounds computed correctly, but rewrites stored the original EWKB bytes.
What changed
EWKB values are converted before the write, while valid ISO WKB values and nulls are preserved. The input batch is not mutated, and the original byte order is retained.
Tests