fix(table): accept EWKB-encoded geometry values when computing geo bounds - #1598
fix(table): accept EWKB-encoded geometry values when computing geo bounds#1598twuebi wants to merge 1 commit into
Conversation
…unds Iceberg prescribes ISO WKB, but geometry values with EWKB type codes (dimension flags in the high bits, an optional embedded SRID) exist in the wild, and the ISO-only decoder used for bounds failed with "wkb: unknown type: 2147483649", aborting the entire file rewrite. Dispatch on the flag bits so both encodings decode. Coordinates are identical either way, so bounds are encoding-independent, and the stored bytes pass through unmodified.
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
Nice, this is a clean fix — sniffing the EWKB flags and routing to the right decoder is the right call, and keeping the stored bytes untouched (read-only, stats-only) is exactly right given the rewrite path shouldn't be re-encoding geometry on the fly. The coordinate-equivalence tests between the two encodings are a good way to pin it.
The fix itself I'm happy with; my asks are all on the test side, plus one thing worth noting in the PR description.
The tests declare their own copies of the EWKB flag constants right next to the production ones — since the test is in package internal it can just use ewkbFlagZ/M/SRID directly, otherwise a future correction to those values leaves the tests silently green against stale bits. And the geography case passes vacuously today: comparing nil bounds to nil bounds doesn't prove the value was actually decoded, so I'd assert the accumulator really consumed it. I'd also add a GeometryCollection case, since that's the one path that exercises recursive sub-geometry decoding.
Worth a line in the PR description too: this fixes the read/stats path, but it doesn't normalize the stored bytes, so a table compacted by iceberg-go still carries EWKB in its data files. Fully closing the gap is a write-path normalization follow-up — just want it clear this is the read-side half, not the whole thing.
Tighten the tests and I'm happy to merge.
| wkbPointZM = 3001 | ||
| wkbLineStringZ = 1002 | ||
|
|
||
| ewkbTestZ = 0x80000000 |
There was a problem hiding this comment.
The test declares its own ewkbTestZ/M/SRID right next to production's ewkbFlagZ/M/SRID with identical values. Since this file is package internal it can reference the production constants directly — I'd drop the test copies and use ewkbFlagZ|ewkbFlagM|ewkbFlagSRID. As it stands, if someone ever corrects a flag value in geo_codec.go the tests stay green against the stale bits, so they'd be validating the wrong pattern.
Same story with the 0x01/0x00 byte-order markers in newWKBBuilder/newXDRWKBBuilder just below — those are wkbLittleEndian/wkbBigEndian in production. wdyt?
| assert.Equal(t, isoLower, ewkbLower) | ||
| assert.Equal(t, isoUpper, ewkbUpper) | ||
| if tt.geograph { | ||
| assert.Nil(t, ewkbLower, "geography bounds must be omitted") |
There was a problem hiding this comment.
For the geography case, assert.Equal(isoLower, ewkbLower) is comparing nil to nil and this assert.Nil just confirms it stayed nil — so this passes even if AddWKB decoded nothing at all. The require.NoError above catches a hard decode error, but not a decode that silently succeeds without extending anything.
I'd assert the accumulator actually consumed the value (its internal geom/coordinate count moved past zero) after the add, so the case distinguishes "geography suppresses bounds" from "the value was quietly ignored." Otherwise it stays green if decodeWKB ever starts returning early.
| wantLength: 24, | ||
| }, | ||
| { | ||
| name: "ewkb linestring z with srid", |
There was a problem hiding this comment.
The table covers points and a linestring but not a GeometryCollection, which is exactly where the recursive path matters — ewkb.Unmarshal decodes each sub-geometry, and Trino/PostGIS emit these regularly. I'd add one case (EWKB collection with a Z flag) so a future go-geom bump can't silently regress nested decoding.
While you're there, worth pinning the mixed-encoding behavior: isEWKB only sniffs the outer type word, so an EWKB-flagged outer with ISO-encoded sub-geometries (or the reverse) hits the wrong decoder and errors. That's a fine failure mode — it aborts rather than corrupting bounds — but a case that documents "mixed encoding within a collection is unsupported, and errors" makes the boundary explicit.
| return wkb.Unmarshal(data) | ||
| } | ||
|
|
||
| // isEWKB reports whether data's type word carries EWKB flags. The two decoders |
There was a problem hiding this comment.
The comment explains why the decoders aren't interchangeable for flagged and ISO-dimension values, but not the plain-2D case: a 2D EWKB value carries no flags, so isEWKB returns false and it goes to the ISO decoder — which is correct only because 2D EWKB is byte-identical to ISO WKB. That invariant is load-bearing and currently unstated. I'd add a sentence so nobody later "tightens" the heuristic and breaks the 2D path.
|
|
||
| // EWKB dimension and SRID flags, carried in the high bits of the WKB type word. | ||
| const ( | ||
| ewkbFlagZ = 0x80000000 |
There was a problem hiding this comment.
Minor, but I'd type these uint32 explicitly (const ewkbFlagZ uint32 = 0x80000000). 0x80000000 is above MaxInt32, so today it only works because typeWord is uint32 and the untyped constant resolves against it — the moment one of these lands in a signed-int context it's a compile error on 32-bit. Typing them makes the intent obvious at the declaration too.
2 participants
Iceberg prescribes ISO WKB, but geometry values with EWKB type codes (dimension flags in the high bits, an optional embedded SRID) exist in the wild (Trino 483), and the ISO-only decoder used for bounds failed with "wkb: unknown type: 2147483649", aborting the entire file rewrite.
Dispatch on the flag bits so both encodings decode. Coordinates are identical either way, so bounds are encoding-independent, and the stored bytes pass through unmodified.