laskoviymishka commented on code in PR #1598:
URL: https://github.com/apache/iceberg-go/pull/1598#discussion_r3678326317
##########
table/internal/geo_codec_internal_test.go:
##########
@@ -237,6 +237,278 @@ func TestGeoBoundsAccumulatorInvalidWKB(t *testing.T) {
assert.Error(t, acc.AddWKB([]byte{0x01, 0x02, 0x03}))
}
+// WKB type words used by the EWKB tests below. ISO WKB encodes the dimension
in
+// the type value itself (PointZ = 1001), while EWKB sets flags in the high
bits
+// of the type word and optionally embeds an SRID after it.
+const (
+ wkbPoint = 1
+ wkbLineString = 2
+ wkbPointZ = 1001
+ wkbPointM = 2001
+ wkbPointZM = 3001
+ wkbLineStringZ = 1002
+
+ ewkbTestZ = 0x80000000
Review 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?
##########
table/internal/geo_codec_internal_test.go:
##########
@@ -237,6 +237,278 @@ func TestGeoBoundsAccumulatorInvalidWKB(t *testing.T) {
assert.Error(t, acc.AddWKB([]byte{0x01, 0x02, 0x03}))
}
+// WKB type words used by the EWKB tests below. ISO WKB encodes the dimension
in
+// the type value itself (PointZ = 1001), while EWKB sets flags in the high
bits
+// of the type word and optionally embeds an SRID after it.
+const (
+ wkbPoint = 1
+ wkbLineString = 2
+ wkbPointZ = 1001
+ wkbPointM = 2001
+ wkbPointZM = 3001
+ wkbLineStringZ = 1002
+
+ ewkbTestZ = 0x80000000
+ ewkbTestM = 0x40000000
+ ewkbTestSRID = 0x20000000
+)
+
+// wkbBuilder assembles a WKB value byte by byte: the byte-order marker, then
+// uint32 headers and float64 coordinates in that byte order.
+type wkbBuilder struct {
+ buf []byte
+ order binary.AppendByteOrder
+}
+
+// newWKBBuilder builds a little-endian (NDR) value.
+func newWKBBuilder(typeWord uint32) *wkbBuilder {
+ return (&wkbBuilder{buf: []byte{0x01}, order:
binary.LittleEndian}).u32(typeWord)
+}
+
+// newXDRWKBBuilder builds a big-endian (XDR) value.
+func newXDRWKBBuilder(typeWord uint32) *wkbBuilder {
+ return (&wkbBuilder{buf: []byte{0x00}, order:
binary.BigEndian}).u32(typeWord)
+}
+
+func (b *wkbBuilder) u32(v uint32) *wkbBuilder {
+ b.buf = b.order.AppendUint32(b.buf, v)
+
+ return b
+}
+
+func (b *wkbBuilder) f64(vals ...float64) *wkbBuilder {
+ for _, v := range vals {
+ b.buf = b.order.AppendUint64(b.buf, math.Float64bits(v))
+ }
+
+ return b
+}
+
+func (b *wkbBuilder) bytes() []byte { return b.buf }
+
+// assertCoords compares bound coordinates treating NaN as equal to NaN, which
+// assert.Equal does not (the XYM bound carries NaN in its Z slot).
+func assertCoords(t *testing.T, want, got []float64) {
+ t.Helper()
+ require.Len(t, got, len(want))
+ for i := range want {
+ if math.IsNaN(want[i]) {
+ assert.True(t, math.IsNaN(got[i]), "coord %d: want NaN,
got %v", i, got[i])
+
+ continue
+ }
+ assert.Equal(t, want[i], got[i], "coord %d", i)
+ }
+}
+
+// TestGeoBoundsAccumulatorEWKB verifies that bounds are computed from both ISO
+// WKB (as Iceberg prescribes) and EWKB-flagged values, which some writers
emit:
+// dimension flags in the high bits of the type word, with an optional embedded
+// SRID that is irrelevant to the bounding box.
+func TestGeoBoundsAccumulatorEWKB(t *testing.T) {
+ tests := []struct {
+ name string
+ wkb []byte
+ wantLower []float64
+ wantUpper []float64
+ wantLength int
+ }{
+ {
+ name: "iso point xy",
+ wkb: newWKBBuilder(wkbPoint).f64(1, 2).bytes(),
+ wantLower: []float64{1, 2},
+ wantUpper: []float64{1, 2},
+ wantLength: 16,
+ },
+ {
+ name: "iso point z",
+ wkb: newWKBBuilder(wkbPointZ).f64(1, 2,
3).bytes(),
+ wantLower: []float64{1, 2, 3},
+ wantUpper: []float64{1, 2, 3},
+ wantLength: 24,
+ },
+ {
+ name: "ewkb point z",
+ wkb: newWKBBuilder(wkbPoint|ewkbTestZ).f64(1, 2,
3).bytes(),
+ wantLower: []float64{1, 2, 3},
+ wantUpper: []float64{1, 2, 3},
+ wantLength: 24,
+ },
+ {
+ name: "ewkb point z with srid",
+ wkb:
newWKBBuilder(wkbPoint|ewkbTestZ|ewkbTestSRID).u32(4326).f64(1, 2, 3).bytes(),
+ wantLower: []float64{1, 2, 3},
+ wantUpper: []float64{1, 2, 3},
+ wantLength: 24,
+ },
+ {
+ name: "ewkb point xy with srid",
+ wkb:
newWKBBuilder(wkbPoint|ewkbTestSRID).u32(4326).f64(1, 2).bytes(),
+ wantLower: []float64{1, 2},
+ wantUpper: []float64{1, 2},
+ wantLength: 16,
+ },
+ {
+ name: "iso point m",
+ wkb: newWKBBuilder(wkbPointM).f64(1, 2,
100).bytes(),
+ wantLower: []float64{1, 2, math.NaN(), 100},
+ wantUpper: []float64{1, 2, math.NaN(), 100},
+ wantLength: 32,
+ },
+ {
+ name: "ewkb point m",
+ wkb: newWKBBuilder(wkbPoint|ewkbTestM).f64(1, 2,
100).bytes(),
+ wantLower: []float64{1, 2, math.NaN(), 100},
+ wantUpper: []float64{1, 2, math.NaN(), 100},
+ wantLength: 32,
+ },
+ {
+ name: "iso point zm",
+ wkb: newWKBBuilder(wkbPointZM).f64(1, 2, 3,
100).bytes(),
+ wantLower: []float64{1, 2, 3, 100},
+ wantUpper: []float64{1, 2, 3, 100},
+ wantLength: 32,
+ },
+ {
+ name: "ewkb point zm",
+ wkb:
newWKBBuilder(wkbPoint|ewkbTestZ|ewkbTestM).f64(1, 2, 3, 100).bytes(),
+ wantLower: []float64{1, 2, 3, 100},
+ wantUpper: []float64{1, 2, 3, 100},
+ wantLength: 32,
+ },
+ {
+ name: "ewkb linestring z",
+ wkb:
newWKBBuilder(wkbLineString|ewkbTestZ).u32(2).f64(1, 2, 3, 4, 5, 6).bytes(),
+ wantLower: []float64{1, 2, 3},
+ wantUpper: []float64{4, 5, 6},
+ wantLength: 24,
+ },
+ {
+ name: "ewkb point z big endian",
+ wkb: newXDRWKBBuilder(wkbPoint|ewkbTestZ).f64(1,
2, 3).bytes(),
+ wantLower: []float64{1, 2, 3},
+ wantUpper: []float64{1, 2, 3},
+ wantLength: 24,
+ },
+ {
+ name: "ewkb linestring z with srid",
Review 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.
##########
table/internal/geo_codec.go:
##########
@@ -87,6 +88,57 @@ func (a *geoBoundsAccumulator) AddWKB(data []byte) error {
return nil
}
+// EWKB dimension and SRID flags, carried in the high bits of the WKB type
word.
+const (
+ ewkbFlagZ = 0x80000000
Review 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.
##########
table/internal/geo_codec.go:
##########
@@ -87,6 +88,57 @@ func (a *geoBoundsAccumulator) AddWKB(data []byte) error {
return nil
}
+// EWKB dimension and SRID flags, carried in the high bits of the WKB type
word.
+const (
+ ewkbFlagZ = 0x80000000
+ ewkbFlagM = 0x40000000
+ ewkbFlagSRID = 0x20000000
+ ewkbFlags = ewkbFlagZ | ewkbFlagM | ewkbFlagSRID
+)
+
+// WKB byte-order markers, the first byte of every WKB value.
+const (
+ wkbBigEndian = 0
+ wkbLittleEndian = 1
+)
+
+// decodeWKB decodes a geospatial value for statistics purposes, accepting both
+// encodings found in the wild. Iceberg prescribes ISO WKB, which encodes the
+// dimension in the type value itself (PointZ = 1001); some writers instead
emit
+// EWKB, which flags Z/M in the high bits of the type word and may embed an
SRID
+// after it. Both yield the same coordinates, so bounds do not depend on the
+// encoding. Stored values are never rewritten - this decode is read-only.
+func decodeWKB(data []byte) (geom.T, error) {
+ if isEWKB(data) {
+ return ewkb.Unmarshal(data)
+ }
+
+ return wkb.Unmarshal(data)
+}
+
+// isEWKB reports whether data's type word carries EWKB flags. The two decoders
Review 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.
##########
table/internal/geo_codec_internal_test.go:
##########
@@ -237,6 +237,278 @@ func TestGeoBoundsAccumulatorInvalidWKB(t *testing.T) {
assert.Error(t, acc.AddWKB([]byte{0x01, 0x02, 0x03}))
}
+// WKB type words used by the EWKB tests below. ISO WKB encodes the dimension
in
+// the type value itself (PointZ = 1001), while EWKB sets flags in the high
bits
+// of the type word and optionally embeds an SRID after it.
+const (
+ wkbPoint = 1
+ wkbLineString = 2
+ wkbPointZ = 1001
+ wkbPointM = 2001
+ wkbPointZM = 3001
+ wkbLineStringZ = 1002
+
+ ewkbTestZ = 0x80000000
+ ewkbTestM = 0x40000000
+ ewkbTestSRID = 0x20000000
+)
+
+// wkbBuilder assembles a WKB value byte by byte: the byte-order marker, then
+// uint32 headers and float64 coordinates in that byte order.
+type wkbBuilder struct {
+ buf []byte
+ order binary.AppendByteOrder
+}
+
+// newWKBBuilder builds a little-endian (NDR) value.
+func newWKBBuilder(typeWord uint32) *wkbBuilder {
+ return (&wkbBuilder{buf: []byte{0x01}, order:
binary.LittleEndian}).u32(typeWord)
+}
+
+// newXDRWKBBuilder builds a big-endian (XDR) value.
+func newXDRWKBBuilder(typeWord uint32) *wkbBuilder {
+ return (&wkbBuilder{buf: []byte{0x00}, order:
binary.BigEndian}).u32(typeWord)
+}
+
+func (b *wkbBuilder) u32(v uint32) *wkbBuilder {
+ b.buf = b.order.AppendUint32(b.buf, v)
+
+ return b
+}
+
+func (b *wkbBuilder) f64(vals ...float64) *wkbBuilder {
+ for _, v := range vals {
+ b.buf = b.order.AppendUint64(b.buf, math.Float64bits(v))
+ }
+
+ return b
+}
+
+func (b *wkbBuilder) bytes() []byte { return b.buf }
+
+// assertCoords compares bound coordinates treating NaN as equal to NaN, which
+// assert.Equal does not (the XYM bound carries NaN in its Z slot).
+func assertCoords(t *testing.T, want, got []float64) {
+ t.Helper()
+ require.Len(t, got, len(want))
+ for i := range want {
+ if math.IsNaN(want[i]) {
+ assert.True(t, math.IsNaN(got[i]), "coord %d: want NaN,
got %v", i, got[i])
+
+ continue
+ }
+ assert.Equal(t, want[i], got[i], "coord %d", i)
+ }
+}
+
+// TestGeoBoundsAccumulatorEWKB verifies that bounds are computed from both ISO
+// WKB (as Iceberg prescribes) and EWKB-flagged values, which some writers
emit:
+// dimension flags in the high bits of the type word, with an optional embedded
+// SRID that is irrelevant to the bounding box.
+func TestGeoBoundsAccumulatorEWKB(t *testing.T) {
+ tests := []struct {
+ name string
+ wkb []byte
+ wantLower []float64
+ wantUpper []float64
+ wantLength int
+ }{
+ {
+ name: "iso point xy",
+ wkb: newWKBBuilder(wkbPoint).f64(1, 2).bytes(),
+ wantLower: []float64{1, 2},
+ wantUpper: []float64{1, 2},
+ wantLength: 16,
+ },
+ {
+ name: "iso point z",
+ wkb: newWKBBuilder(wkbPointZ).f64(1, 2,
3).bytes(),
+ wantLower: []float64{1, 2, 3},
+ wantUpper: []float64{1, 2, 3},
+ wantLength: 24,
+ },
+ {
+ name: "ewkb point z",
+ wkb: newWKBBuilder(wkbPoint|ewkbTestZ).f64(1, 2,
3).bytes(),
+ wantLower: []float64{1, 2, 3},
+ wantUpper: []float64{1, 2, 3},
+ wantLength: 24,
+ },
+ {
+ name: "ewkb point z with srid",
+ wkb:
newWKBBuilder(wkbPoint|ewkbTestZ|ewkbTestSRID).u32(4326).f64(1, 2, 3).bytes(),
+ wantLower: []float64{1, 2, 3},
+ wantUpper: []float64{1, 2, 3},
+ wantLength: 24,
+ },
+ {
+ name: "ewkb point xy with srid",
+ wkb:
newWKBBuilder(wkbPoint|ewkbTestSRID).u32(4326).f64(1, 2).bytes(),
+ wantLower: []float64{1, 2},
+ wantUpper: []float64{1, 2},
+ wantLength: 16,
+ },
+ {
+ name: "iso point m",
+ wkb: newWKBBuilder(wkbPointM).f64(1, 2,
100).bytes(),
+ wantLower: []float64{1, 2, math.NaN(), 100},
+ wantUpper: []float64{1, 2, math.NaN(), 100},
+ wantLength: 32,
+ },
+ {
+ name: "ewkb point m",
+ wkb: newWKBBuilder(wkbPoint|ewkbTestM).f64(1, 2,
100).bytes(),
+ wantLower: []float64{1, 2, math.NaN(), 100},
+ wantUpper: []float64{1, 2, math.NaN(), 100},
+ wantLength: 32,
+ },
+ {
+ name: "iso point zm",
+ wkb: newWKBBuilder(wkbPointZM).f64(1, 2, 3,
100).bytes(),
+ wantLower: []float64{1, 2, 3, 100},
+ wantUpper: []float64{1, 2, 3, 100},
+ wantLength: 32,
+ },
+ {
+ name: "ewkb point zm",
+ wkb:
newWKBBuilder(wkbPoint|ewkbTestZ|ewkbTestM).f64(1, 2, 3, 100).bytes(),
+ wantLower: []float64{1, 2, 3, 100},
+ wantUpper: []float64{1, 2, 3, 100},
+ wantLength: 32,
+ },
+ {
+ name: "ewkb linestring z",
+ wkb:
newWKBBuilder(wkbLineString|ewkbTestZ).u32(2).f64(1, 2, 3, 4, 5, 6).bytes(),
+ wantLower: []float64{1, 2, 3},
+ wantUpper: []float64{4, 5, 6},
+ wantLength: 24,
+ },
+ {
+ name: "ewkb point z big endian",
+ wkb: newXDRWKBBuilder(wkbPoint|ewkbTestZ).f64(1,
2, 3).bytes(),
+ wantLower: []float64{1, 2, 3},
+ wantUpper: []float64{1, 2, 3},
+ wantLength: 24,
+ },
+ {
+ name: "ewkb linestring z with srid",
+ wkb:
newWKBBuilder(wkbLineString|ewkbTestZ|ewkbTestSRID).u32(4326).u32(2).f64(1, 2,
3, 4, 5, 6).bytes(),
+ wantLower: []float64{1, 2, 3},
+ wantUpper: []float64{4, 5, 6},
+ wantLength: 24,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ acc := newGeoBoundsAccumulator(false)
+ require.NoError(t, acc.AddWKB(tt.wkb))
+
+ lower, upper := acc.Bounds()
+ require.Len(t, lower, tt.wantLength)
+ require.Len(t, upper, tt.wantLength)
+
+ assertCoords(t, tt.wantLower, decodeBound(t, lower))
+ assertCoords(t, tt.wantUpper, decodeBound(t, upper))
+ })
+ }
+}
+
+// TestGeoBoundsAccumulatorEWKBMatchesISO verifies that the two encodings of
the
+// same coordinates produce byte-identical bounds, so a file's statistics do
not
+// depend on which encoding its writer used.
+func TestGeoBoundsAccumulatorEWKBMatchesISO(t *testing.T) {
+ tests := []struct {
+ name string
+ iso []byte
+ ewkb []byte
+ geograph bool
+ }{
+ {
+ name: "point xy",
+ iso: newWKBBuilder(wkbPoint).f64(1, 2).bytes(),
+ ewkb:
newWKBBuilder(wkbPoint|ewkbTestSRID).u32(4326).f64(1, 2).bytes(),
+ },
+ {
+ name: "point z",
+ iso: newWKBBuilder(wkbPointZ).f64(1, 2, 3).bytes(),
+ ewkb: newWKBBuilder(wkbPoint|ewkbTestZ).f64(1, 2,
3).bytes(),
+ },
+ {
+ name: "point m",
+ iso: newWKBBuilder(wkbPointM).f64(1, 2, 100).bytes(),
+ ewkb: newWKBBuilder(wkbPoint|ewkbTestM).f64(1, 2,
100).bytes(),
+ },
+ {
+ name: "point zm",
+ iso: newWKBBuilder(wkbPointZM).f64(1, 2, 3,
100).bytes(),
+ ewkb:
newWKBBuilder(wkbPoint|ewkbTestZ|ewkbTestM).f64(1, 2, 3, 100).bytes(),
+ },
+ {
+ name: "point z big endian",
+ iso: newWKBBuilder(wkbPointZ).f64(1, 2, 3).bytes(),
+ ewkb: newXDRWKBBuilder(wkbPoint|ewkbTestZ).f64(1, 2,
3).bytes(),
+ },
+ {
+ name: "linestring z",
+ iso: newWKBBuilder(wkbLineStringZ).u32(2).f64(1, 2, 3,
4, 5, 6).bytes(),
+ ewkb:
newWKBBuilder(wkbLineString|ewkbTestZ).u32(2).f64(1, 2, 3, 4, 5, 6).bytes(),
+ },
+ {
+ // Geography emits no bounds for either encoding, but
the value must
+ // still decode: a decode error aborts the whole file
rewrite.
+ name: "geography point z",
+ iso: newWKBBuilder(wkbPointZ).f64(1, 2, 3).bytes(),
+ ewkb: newWKBBuilder(wkbPoint|ewkbTestZ).f64(1, 2,
3).bytes(),
+ geograph: true,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ isoAcc := newGeoBoundsAccumulator(tt.geograph)
+ require.NoError(t, isoAcc.AddWKB(tt.iso))
+ isoLower, isoUpper := isoAcc.Bounds()
+
+ ewkbAcc := newGeoBoundsAccumulator(tt.geograph)
+ require.NoError(t, ewkbAcc.AddWKB(tt.ewkb))
+ ewkbLower, ewkbUpper := ewkbAcc.Bounds()
+
+ assert.Equal(t, isoLower, ewkbLower)
+ assert.Equal(t, isoUpper, ewkbUpper)
+ if tt.geograph {
+ assert.Nil(t, ewkbLower, "geography bounds must
be omitted")
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
