Skip to content

fix(table): normalize EWKB geometry values on write - #1670

Draft
fallintoplace wants to merge 9 commits into
apache:mainfrom
fallintoplace:fix/ewkb-write-normalization
Draft

fix(table): normalize EWKB geometry values on write#1670
fallintoplace wants to merge 9 commits into
apache:mainfrom
fallintoplace:fix/ewkb-write-normalization

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

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

  • go test ./table/internal -count=1

@fallintoplace
fallintoplace marked this pull request as draft August 6, 2026 19:49
@fallintoplace
fallintoplace marked this pull request as ready for review August 6, 2026 19:51
@fallintoplace
fallintoplace marked this pull request as draft August 6, 2026 20:59
@fallintoplace fallintoplace reopened this Aug 12, 2026

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Per-batch overhead for non-geo schemas. normalizeGeoBatch recurses into every column of every batch via normalizeNestedArrayReachable (table/internal/parquet_files.go). For any nested column — geo or not — normalizeListChild allocates a childActive []bool sized to the full child value range, slices the values array, and walks all offsets, on every Write call. 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's arrow.DataType contain a *geoarrow.WKBType anywhere — computable once per writer, or at least once per column type) would make the common no-geo case zero-cost. Since parquetFormat.NewFileWriter already inspects the schema for WKB types around line 404, the writer could cache which column indices even need visiting.

  2. Undocumented behavior change on the read/bounds path. decodeWKB now passes EmptyPointHandlingNaN to wkb.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's ewkb.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.

  3. 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 accumulateGeoBounds makes 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants