laskoviymishka commented on code in PR #1706:
URL: https://github.com/apache/iceberg-go/pull/1706#discussion_r3740297780
##########
table/geo_write_test.go:
##########
@@ -48,34 +55,20 @@ func wktToWKB(t *testing.T, s string) geoarrow.WKBBytes {
return geoarrow.WKBBytes(b)
}
-// TestWriteGeometryColumnPopulatesBounds writes a geometry and a geography
-// column end-to-end through the data file writer and checks the manifest-level
-// geo bounds that iceberg-go computes from the WKB values. arrow-go's Parquet
-// writer does not emit native GeoStatistics, so iceberg-go derives the column
-// bounds itself (see geoBoundsAccumulator) and threads them into the DataFile
-// exactly as the manifest carries them. Geometry gets a planar XY bounding
box;
-// geography stays unbounded because a planar box over geodesic edges is
unsafe.
-//
-// This is the public-path counterpart to internal.TestWriteDataFileGeoBounds:
-// that test feeds hand-built StatsCols straight to WriteDataFile, whereas this
-// one drives the full table writer (newDataFileWriter/writeFile/WriteTask
over a
-// MetadataBuilder), so it also covers geo stats-collector setup in the path a
-// caller actually uses.
-func TestWriteGeometryColumnPopulatesBounds(t *testing.T) {
- t.Parallel()
-
- const (
- geomFieldID = 2
- geogFieldID = 3
- )
+// newGeoTestWriter builds an unpartitioned v3 table with an id/geom/geog
schema
+// and returns a data file writer over it. props feed the metrics-mode
+// configuration (e.g. write.metadata.metrics.column.geom=none) so tests can
+// exercise the stats-plan dispatch that decides whether geo bounds are
recorded.
Review Comment:
`newGeoTestWriter` only builds top-level geo columns, and nested geo columns
are still unhandled in the writer (the `TODO(#992)` over in
`parquet_files.go`). A one-line note in this docstring that nested geo isn't
covered here would stop someone reading only the test file from assuming it is.
##########
table/geo_write_test.go:
##########
@@ -118,17 +142,145 @@ func TestWriteGeometryColumnPopulatesBounds(t
*testing.T) {
// Geometry column carries a planar XY bounding box in the manifest
bounds.
lower := df.LowerBoundValues()
upper := df.UpperBoundValues()
- require.Contains(t, lower, geomFieldID, "geometry column must record a
lower bound")
- require.Contains(t, upper, geomFieldID, "geometry column must record an
upper bound")
+ require.Contains(t, lower, geoTestGeomFieldID, "geometry column must
record a lower bound")
+ require.Contains(t, upper, geoTestGeomFieldID, "geometry column must
record an upper bound")
- minX, minY, maxX, maxY, ok := tblutils.GeoBoundsXY(lower[geomFieldID],
upper[geomFieldID])
+ minX, minY, maxX, maxY, ok :=
tblutils.GeoBoundsXY(lower[geoTestGeomFieldID], upper[geoTestGeomFieldID])
require.True(t, ok, "geometry bounds must decode to a planar XY box")
assert.Equal(t, 0.0, minX)
assert.Equal(t, -5.0, minY)
assert.Equal(t, 30.0, maxX)
assert.Equal(t, 10.0, maxY)
- // Geography stays unbounded: a planar box over geodesic edges is
unsafe.
- assert.NotContains(t, lower, geogFieldID, "geography column must not
record bounds")
- assert.NotContains(t, upper, geogFieldID, "geography column must not
record bounds")
+ // The non-geo id column still gets ordinary min/max bounds. The
geo-type guard
+ // in DataFileStatsFromMeta suppresses generic Parquet stats for geo
columns;
+ // pinning id here catches a regression that over-suppresses an adjacent
+ // non-geo column.
+ require.Contains(t, lower, geoTestIDFieldID, "non-geo id column must
still record a lower bound")
+ require.Contains(t, upper, geoTestIDFieldID, "non-geo id column must
still record an upper bound")
+
+ // Geography does not record bounds in the current implementation: the
V3 spec
+ // permits geography bounds (with the xmin > xmax antimeridian-wrapping
+ // convention), but iceberg-go leaves them unbounded as a deliberate
+ // conservative choice until geodesic/antimeridian-aware computation
lands (see
+ // geoBoundsAccumulator). This assertion should flip when that
computation is
+ // added.
+ assert.NotContains(t, lower, geoTestGeogFieldID, "geography column does
not record bounds in the current implementation")
+ assert.NotContains(t, upper, geoTestGeogFieldID, "geography column does
not record bounds in the current implementation")
+}
+
+// TestWriteGeometryColumnAllNull writes a geometry column whose values are all
+// null and asserts the field drops out of the manifest bounds map through the
+// full write path. With no WKB values to accumulate, geoBoundsAccumulator has
+// nothing to encode, so no bound must be emitted.
+func TestWriteGeometryColumnAllNull(t *testing.T) {
+ t.Parallel()
+
+ writer, schema, arrowSchema := newGeoTestWriter(t, t.TempDir(),
iceberg.Properties{})
+
+ rec, _, err := array.RecordFromJSON(memory.DefaultAllocator,
arrowSchema, strings.NewReader(`[
+ {"id": 1, "geom": null, "geog": null},
+ {"id": 2, "geom": null, "geog": null}
+ ]`))
+ require.NoError(t, err)
+ defer rec.Release()
+
+ df, err := writer.writeFile(t.Context(), nil, WriteTask{
+ Uuid: uuid.New(),
+ ID: 0,
+ FileCount: 1,
+ Schema: schema,
+ Batches: []arrow.RecordBatch{rec},
+ })
+ require.NoError(t, err)
+ require.EqualValues(t, 2, df.Count())
+
+ lower := df.LowerBoundValues()
+ upper := df.UpperBoundValues()
+ assert.NotContains(t, lower, geoTestGeomFieldID, "all-null geometry
column must not record a lower bound")
Review Comment:
this passes both when the accumulator ran and found nothing, and when the
accumulator was never wired up at all (say `collectGeoColumns` stopped
registering geo columns), which is the more interesting regression.
Fine to leave since `PopulatesBounds` is the positive guard against the
bypass case, but I'd add a one-line comment here pointing at that so the intent
is clear. wdyt?
##########
table/geo_write_test.go:
##########
@@ -118,17 +142,145 @@ func TestWriteGeometryColumnPopulatesBounds(t
*testing.T) {
// Geometry column carries a planar XY bounding box in the manifest
bounds.
lower := df.LowerBoundValues()
upper := df.UpperBoundValues()
- require.Contains(t, lower, geomFieldID, "geometry column must record a
lower bound")
- require.Contains(t, upper, geomFieldID, "geometry column must record an
upper bound")
+ require.Contains(t, lower, geoTestGeomFieldID, "geometry column must
record a lower bound")
+ require.Contains(t, upper, geoTestGeomFieldID, "geometry column must
record an upper bound")
- minX, minY, maxX, maxY, ok := tblutils.GeoBoundsXY(lower[geomFieldID],
upper[geomFieldID])
+ minX, minY, maxX, maxY, ok :=
tblutils.GeoBoundsXY(lower[geoTestGeomFieldID], upper[geoTestGeomFieldID])
require.True(t, ok, "geometry bounds must decode to a planar XY box")
assert.Equal(t, 0.0, minX)
assert.Equal(t, -5.0, minY)
assert.Equal(t, 30.0, maxX)
assert.Equal(t, 10.0, maxY)
- // Geography stays unbounded: a planar box over geodesic edges is
unsafe.
- assert.NotContains(t, lower, geogFieldID, "geography column must not
record bounds")
- assert.NotContains(t, upper, geogFieldID, "geography column must not
record bounds")
+ // The non-geo id column still gets ordinary min/max bounds. The
geo-type guard
+ // in DataFileStatsFromMeta suppresses generic Parquet stats for geo
columns;
+ // pinning id here catches a regression that over-suppresses an adjacent
+ // non-geo column.
+ require.Contains(t, lower, geoTestIDFieldID, "non-geo id column must
still record a lower bound")
+ require.Contains(t, upper, geoTestIDFieldID, "non-geo id column must
still record an upper bound")
+
+ // Geography does not record bounds in the current implementation: the
V3 spec
+ // permits geography bounds (with the xmin > xmax antimeridian-wrapping
+ // convention), but iceberg-go leaves them unbounded as a deliberate
+ // conservative choice until geodesic/antimeridian-aware computation
lands (see
+ // geoBoundsAccumulator). This assertion should flip when that
computation is
+ // added.
+ assert.NotContains(t, lower, geoTestGeogFieldID, "geography column does
not record bounds in the current implementation")
+ assert.NotContains(t, upper, geoTestGeogFieldID, "geography column does
not record bounds in the current implementation")
+}
+
+// TestWriteGeometryColumnAllNull writes a geometry column whose values are all
+// null and asserts the field drops out of the manifest bounds map through the
+// full write path. With no WKB values to accumulate, geoBoundsAccumulator has
+// nothing to encode, so no bound must be emitted.
+func TestWriteGeometryColumnAllNull(t *testing.T) {
+ t.Parallel()
+
+ writer, schema, arrowSchema := newGeoTestWriter(t, t.TempDir(),
iceberg.Properties{})
+
+ rec, _, err := array.RecordFromJSON(memory.DefaultAllocator,
arrowSchema, strings.NewReader(`[
+ {"id": 1, "geom": null, "geog": null},
+ {"id": 2, "geom": null, "geog": null}
+ ]`))
+ require.NoError(t, err)
+ defer rec.Release()
+
+ df, err := writer.writeFile(t.Context(), nil, WriteTask{
+ Uuid: uuid.New(),
+ ID: 0,
+ FileCount: 1,
+ Schema: schema,
+ Batches: []arrow.RecordBatch{rec},
+ })
+ require.NoError(t, err)
+ require.EqualValues(t, 2, df.Count())
+
+ lower := df.LowerBoundValues()
+ upper := df.UpperBoundValues()
+ assert.NotContains(t, lower, geoTestGeomFieldID, "all-null geometry
column must not record a lower bound")
+ assert.NotContains(t, upper, geoTestGeomFieldID, "all-null geometry
column must not record an upper bound")
+
+ // The non-geo id column is unaffected by the geometry column being all
null.
+ require.Contains(t, lower, geoTestIDFieldID, "non-geo id column must
still record a lower bound")
+ require.Contains(t, upper, geoTestIDFieldID, "non-geo id column must
still record an upper bound")
+}
+
+// TestWriteGeometryColumnMetricsNone sets
write.metadata.metrics.column.geom=none
+// and asserts computeStatsPlan actually suppresses the geometry bounds. This
+// exercises the stats-plan dispatch (arrowStatsCollector -> applyGeoBounds)
that
+// internal.TestWriteDataFileGeoBounds cannot reach, since that test feeds
+// hand-built StatsCols straight to WriteDataFile. The adjacent non-geo id
column
+// keeps its ordinary bounds so the suppression is scoped to the geometry
column.
+func TestWriteGeometryColumnMetricsNone(t *testing.T) {
+ t.Parallel()
+
+ props := iceberg.Properties{
+ MetricsModeColumnConfPrefix + ".geom":
string(tblutils.MetricModeNone),
+ }
+ writer, schema, arrowSchema := newGeoTestWriter(t, t.TempDir(), props)
+
+ geomLo := wktToWKB(t, "POINT (0 -5)")
+ geomHi := wktToWKB(t, "POINT (30 10)")
+
+ rec, _, err := array.RecordFromJSON(memory.DefaultAllocator,
arrowSchema, strings.NewReader(`[
+ {"id": 1, "geom": "`+geomLo.String()+`", "geog": null},
+ {"id": 2, "geom": "`+geomHi.String()+`", "geog": null}
+ ]`))
+ require.NoError(t, err)
+ defer rec.Release()
+
+ df, err := writer.writeFile(t.Context(), nil, WriteTask{
+ Uuid: uuid.New(),
+ ID: 0,
+ FileCount: 1,
+ Schema: schema,
+ Batches: []arrow.RecordBatch{rec},
+ })
+ require.NoError(t, err)
+ require.EqualValues(t, 2, df.Count())
+
+ lower := df.LowerBoundValues()
+ upper := df.UpperBoundValues()
+ assert.NotContains(t, lower, geoTestGeomFieldID, "geometry column
bounds must be suppressed under metrics mode none")
+ assert.NotContains(t, upper, geoTestGeomFieldID, "geometry column
bounds must be suppressed under metrics mode none")
+
+ // Suppression is scoped to the geom column: id still gets ordinary
bounds.
+ require.Contains(t, lower, geoTestIDFieldID, "non-geo id column must
keep bounds when only geom is set to none")
+ require.Contains(t, upper, geoTestIDFieldID, "non-geo id column must
keep bounds when only geom is set to none")
+}
+
+// TestWriteGeometryColumnCheckedAllocator runs the geometry write path on a
+// checked allocator and asserts zero residual once the writer is done,
catching
+// a silent allocation escape in the geo-bounds accumulator wiring. This
mirrors
+// the memory.NewCheckedAllocator/AssertSize pattern the writer suites use.
+func TestWriteGeometryColumnCheckedAllocator(t *testing.T) {
+ t.Parallel()
+
+ mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
+ defer mem.AssertSize(t, 0)
+ ctx := compute.WithAllocator(t.Context(), mem)
+
+ writer, schema, arrowSchema := newGeoTestWriter(t, t.TempDir(),
iceberg.Properties{})
+
+ geomLo := wktToWKB(t, "POINT (0 -5)")
+ geomHi := wktToWKB(t, "POINT (30 10)")
+ geog := wktToWKB(t, "POINT (12 4)")
+
+ rec, _, err := array.RecordFromJSON(mem, arrowSchema,
strings.NewReader(`[
+ {"id": 1, "geom": "`+geomLo.String()+`", "geog":
"`+geog.String()+`"},
+ {"id": 2, "geom": "`+geomHi.String()+`", "geog": null}
+ ]`))
+ require.NoError(t, err)
+ defer rec.Release()
Review Comment:
`writeFile` owns everything in `task.Batches` and releases each batch itself
(that deferred `b.Release()` loop in `writer.go`), so this defer
double-releases `rec`: `writeFile` drops it to zero before returning, then this
fires and takes it below zero.
Under `DefaultAllocator` that's silent, which is how the other tests get
away with it, but here it corrupts the checked allocator's accounting before
the deferred `AssertSize(t, 0)` runs, so the zero-residual check isn't
measuring what we think it is.
I'd drop this defer, and the same one in the other three tests plus the
original `PopulatesBounds`, since `writeFile` owns the batch in all of them.
##########
table/geo_write_test.go:
##########
@@ -118,17 +142,145 @@ func TestWriteGeometryColumnPopulatesBounds(t
*testing.T) {
// Geometry column carries a planar XY bounding box in the manifest
bounds.
lower := df.LowerBoundValues()
upper := df.UpperBoundValues()
- require.Contains(t, lower, geomFieldID, "geometry column must record a
lower bound")
- require.Contains(t, upper, geomFieldID, "geometry column must record an
upper bound")
+ require.Contains(t, lower, geoTestGeomFieldID, "geometry column must
record a lower bound")
+ require.Contains(t, upper, geoTestGeomFieldID, "geometry column must
record an upper bound")
- minX, minY, maxX, maxY, ok := tblutils.GeoBoundsXY(lower[geomFieldID],
upper[geomFieldID])
+ minX, minY, maxX, maxY, ok :=
tblutils.GeoBoundsXY(lower[geoTestGeomFieldID], upper[geoTestGeomFieldID])
require.True(t, ok, "geometry bounds must decode to a planar XY box")
assert.Equal(t, 0.0, minX)
assert.Equal(t, -5.0, minY)
assert.Equal(t, 30.0, maxX)
assert.Equal(t, 10.0, maxY)
- // Geography stays unbounded: a planar box over geodesic edges is
unsafe.
- assert.NotContains(t, lower, geogFieldID, "geography column must not
record bounds")
- assert.NotContains(t, upper, geogFieldID, "geography column must not
record bounds")
+ // The non-geo id column still gets ordinary min/max bounds. The
geo-type guard
+ // in DataFileStatsFromMeta suppresses generic Parquet stats for geo
columns;
+ // pinning id here catches a regression that over-suppresses an adjacent
+ // non-geo column.
+ require.Contains(t, lower, geoTestIDFieldID, "non-geo id column must
still record a lower bound")
+ require.Contains(t, upper, geoTestIDFieldID, "non-geo id column must
still record an upper bound")
+
+ // Geography does not record bounds in the current implementation: the
V3 spec
+ // permits geography bounds (with the xmin > xmax antimeridian-wrapping
Review Comment:
nice that the comment already calls out the `xmin > xmax` antimeridian
convention.
One thing I'd add while we're here: the geometry decode path (`GeoBoundsXY`)
rejects `xmin > xmax` as inverted, which is correct for geometry but would
silently drop valid antimeridian-crossing geography bounds from Java/PyIceberg
if it ever got reused for the geography pruning path. A short note that
geography will need its own decode would save someone a subtle bug later. Not
blocking, wdyt?
##########
table/geo_write_test.go:
##########
@@ -118,17 +142,145 @@ func TestWriteGeometryColumnPopulatesBounds(t
*testing.T) {
// Geometry column carries a planar XY bounding box in the manifest
bounds.
lower := df.LowerBoundValues()
upper := df.UpperBoundValues()
- require.Contains(t, lower, geomFieldID, "geometry column must record a
lower bound")
- require.Contains(t, upper, geomFieldID, "geometry column must record an
upper bound")
+ require.Contains(t, lower, geoTestGeomFieldID, "geometry column must
record a lower bound")
+ require.Contains(t, upper, geoTestGeomFieldID, "geometry column must
record an upper bound")
- minX, minY, maxX, maxY, ok := tblutils.GeoBoundsXY(lower[geomFieldID],
upper[geomFieldID])
+ minX, minY, maxX, maxY, ok :=
tblutils.GeoBoundsXY(lower[geoTestGeomFieldID], upper[geoTestGeomFieldID])
require.True(t, ok, "geometry bounds must decode to a planar XY box")
assert.Equal(t, 0.0, minX)
assert.Equal(t, -5.0, minY)
assert.Equal(t, 30.0, maxX)
assert.Equal(t, 10.0, maxY)
- // Geography stays unbounded: a planar box over geodesic edges is
unsafe.
- assert.NotContains(t, lower, geogFieldID, "geography column must not
record bounds")
- assert.NotContains(t, upper, geogFieldID, "geography column must not
record bounds")
+ // The non-geo id column still gets ordinary min/max bounds. The
geo-type guard
+ // in DataFileStatsFromMeta suppresses generic Parquet stats for geo
columns;
+ // pinning id here catches a regression that over-suppresses an adjacent
+ // non-geo column.
+ require.Contains(t, lower, geoTestIDFieldID, "non-geo id column must
still record a lower bound")
+ require.Contains(t, upper, geoTestIDFieldID, "non-geo id column must
still record an upper bound")
+
+ // Geography does not record bounds in the current implementation: the
V3 spec
+ // permits geography bounds (with the xmin > xmax antimeridian-wrapping
+ // convention), but iceberg-go leaves them unbounded as a deliberate
+ // conservative choice until geodesic/antimeridian-aware computation
lands (see
+ // geoBoundsAccumulator). This assertion should flip when that
computation is
+ // added.
+ assert.NotContains(t, lower, geoTestGeogFieldID, "geography column does
not record bounds in the current implementation")
+ assert.NotContains(t, upper, geoTestGeogFieldID, "geography column does
not record bounds in the current implementation")
+}
+
+// TestWriteGeometryColumnAllNull writes a geometry column whose values are all
+// null and asserts the field drops out of the manifest bounds map through the
+// full write path. With no WKB values to accumulate, geoBoundsAccumulator has
+// nothing to encode, so no bound must be emitted.
+func TestWriteGeometryColumnAllNull(t *testing.T) {
+ t.Parallel()
+
+ writer, schema, arrowSchema := newGeoTestWriter(t, t.TempDir(),
iceberg.Properties{})
+
+ rec, _, err := array.RecordFromJSON(memory.DefaultAllocator,
arrowSchema, strings.NewReader(`[
+ {"id": 1, "geom": null, "geog": null},
+ {"id": 2, "geom": null, "geog": null}
+ ]`))
+ require.NoError(t, err)
+ defer rec.Release()
+
+ df, err := writer.writeFile(t.Context(), nil, WriteTask{
+ Uuid: uuid.New(),
+ ID: 0,
+ FileCount: 1,
+ Schema: schema,
+ Batches: []arrow.RecordBatch{rec},
+ })
+ require.NoError(t, err)
+ require.EqualValues(t, 2, df.Count())
+
+ lower := df.LowerBoundValues()
+ upper := df.UpperBoundValues()
+ assert.NotContains(t, lower, geoTestGeomFieldID, "all-null geometry
column must not record a lower bound")
+ assert.NotContains(t, upper, geoTestGeomFieldID, "all-null geometry
column must not record an upper bound")
+
+ // The non-geo id column is unaffected by the geometry column being all
null.
+ require.Contains(t, lower, geoTestIDFieldID, "non-geo id column must
still record a lower bound")
+ require.Contains(t, upper, geoTestIDFieldID, "non-geo id column must
still record an upper bound")
+}
+
+// TestWriteGeometryColumnMetricsNone sets
write.metadata.metrics.column.geom=none
+// and asserts computeStatsPlan actually suppresses the geometry bounds. This
+// exercises the stats-plan dispatch (arrowStatsCollector -> applyGeoBounds)
that
+// internal.TestWriteDataFileGeoBounds cannot reach, since that test feeds
+// hand-built StatsCols straight to WriteDataFile. The adjacent non-geo id
column
+// keeps its ordinary bounds so the suppression is scoped to the geometry
column.
+func TestWriteGeometryColumnMetricsNone(t *testing.T) {
+ t.Parallel()
+
+ props := iceberg.Properties{
+ MetricsModeColumnConfPrefix + ".geom":
string(tblutils.MetricModeNone),
+ }
+ writer, schema, arrowSchema := newGeoTestWriter(t, t.TempDir(), props)
+
+ geomLo := wktToWKB(t, "POINT (0 -5)")
+ geomHi := wktToWKB(t, "POINT (30 10)")
+
+ rec, _, err := array.RecordFromJSON(memory.DefaultAllocator,
arrowSchema, strings.NewReader(`[
+ {"id": 1, "geom": "`+geomLo.String()+`", "geog": null},
+ {"id": 2, "geom": "`+geomHi.String()+`", "geog": null}
+ ]`))
+ require.NoError(t, err)
+ defer rec.Release()
+
+ df, err := writer.writeFile(t.Context(), nil, WriteTask{
+ Uuid: uuid.New(),
+ ID: 0,
+ FileCount: 1,
+ Schema: schema,
+ Batches: []arrow.RecordBatch{rec},
+ })
+ require.NoError(t, err)
+ require.EqualValues(t, 2, df.Count())
+
+ lower := df.LowerBoundValues()
+ upper := df.UpperBoundValues()
+ assert.NotContains(t, lower, geoTestGeomFieldID, "geometry column
bounds must be suppressed under metrics mode none")
+ assert.NotContains(t, upper, geoTestGeomFieldID, "geometry column
bounds must be suppressed under metrics mode none")
+
+ // Suppression is scoped to the geom column: id still gets ordinary
bounds.
+ require.Contains(t, lower, geoTestIDFieldID, "non-geo id column must
keep bounds when only geom is set to none")
+ require.Contains(t, upper, geoTestIDFieldID, "non-geo id column must
keep bounds when only geom is set to none")
+}
+
+// TestWriteGeometryColumnCheckedAllocator runs the geometry write path on a
+// checked allocator and asserts zero residual once the writer is done,
catching
Review Comment:
the comment says this catches "a silent allocation escape in the geo-bounds
accumulator wiring," but I don't think the checked allocator can actually see
that. `geoBoundsAccumulator` does its work on plain Go struct fields and hands
back `[]byte` from `encodeGeoBound`, none of which goes through an Arrow
allocator, so a leak there is a GC concern that `AssertSize` never observes.
What this test genuinely validates is that the Arrow write pipeline
(`ToRequestedSchema`, the pqarrow writer) releases its buffers, so I'd reframe
the comment to that. If we do want a real guard on the accumulator holding no
references, that's a separate small unit test on `geoBoundsAccumulator` after
`Bounds()`/`StatsAgg()`. wdyt?
##########
table/geo_write_test.go:
##########
@@ -118,17 +142,145 @@ func TestWriteGeometryColumnPopulatesBounds(t
*testing.T) {
// Geometry column carries a planar XY bounding box in the manifest
bounds.
lower := df.LowerBoundValues()
upper := df.UpperBoundValues()
- require.Contains(t, lower, geomFieldID, "geometry column must record a
lower bound")
- require.Contains(t, upper, geomFieldID, "geometry column must record an
upper bound")
+ require.Contains(t, lower, geoTestGeomFieldID, "geometry column must
record a lower bound")
+ require.Contains(t, upper, geoTestGeomFieldID, "geometry column must
record an upper bound")
- minX, minY, maxX, maxY, ok := tblutils.GeoBoundsXY(lower[geomFieldID],
upper[geomFieldID])
+ minX, minY, maxX, maxY, ok :=
tblutils.GeoBoundsXY(lower[geoTestGeomFieldID], upper[geoTestGeomFieldID])
require.True(t, ok, "geometry bounds must decode to a planar XY box")
assert.Equal(t, 0.0, minX)
assert.Equal(t, -5.0, minY)
assert.Equal(t, 30.0, maxX)
assert.Equal(t, 10.0, maxY)
- // Geography stays unbounded: a planar box over geodesic edges is
unsafe.
- assert.NotContains(t, lower, geogFieldID, "geography column must not
record bounds")
- assert.NotContains(t, upper, geogFieldID, "geography column must not
record bounds")
+ // The non-geo id column still gets ordinary min/max bounds. The
geo-type guard
+ // in DataFileStatsFromMeta suppresses generic Parquet stats for geo
columns;
+ // pinning id here catches a regression that over-suppresses an adjacent
+ // non-geo column.
+ require.Contains(t, lower, geoTestIDFieldID, "non-geo id column must
still record a lower bound")
+ require.Contains(t, upper, geoTestIDFieldID, "non-geo id column must
still record an upper bound")
+
+ // Geography does not record bounds in the current implementation: the
V3 spec
+ // permits geography bounds (with the xmin > xmax antimeridian-wrapping
+ // convention), but iceberg-go leaves them unbounded as a deliberate
+ // conservative choice until geodesic/antimeridian-aware computation
lands (see
+ // geoBoundsAccumulator). This assertion should flip when that
computation is
+ // added.
+ assert.NotContains(t, lower, geoTestGeogFieldID, "geography column does
not record bounds in the current implementation")
+ assert.NotContains(t, upper, geoTestGeogFieldID, "geography column does
not record bounds in the current implementation")
+}
+
+// TestWriteGeometryColumnAllNull writes a geometry column whose values are all
+// null and asserts the field drops out of the manifest bounds map through the
+// full write path. With no WKB values to accumulate, geoBoundsAccumulator has
+// nothing to encode, so no bound must be emitted.
+func TestWriteGeometryColumnAllNull(t *testing.T) {
+ t.Parallel()
+
+ writer, schema, arrowSchema := newGeoTestWriter(t, t.TempDir(),
iceberg.Properties{})
+
+ rec, _, err := array.RecordFromJSON(memory.DefaultAllocator,
arrowSchema, strings.NewReader(`[
+ {"id": 1, "geom": null, "geog": null},
+ {"id": 2, "geom": null, "geog": null}
+ ]`))
+ require.NoError(t, err)
+ defer rec.Release()
+
+ df, err := writer.writeFile(t.Context(), nil, WriteTask{
+ Uuid: uuid.New(),
+ ID: 0,
+ FileCount: 1,
+ Schema: schema,
+ Batches: []arrow.RecordBatch{rec},
+ })
+ require.NoError(t, err)
+ require.EqualValues(t, 2, df.Count())
+
+ lower := df.LowerBoundValues()
+ upper := df.UpperBoundValues()
+ assert.NotContains(t, lower, geoTestGeomFieldID, "all-null geometry
column must not record a lower bound")
+ assert.NotContains(t, upper, geoTestGeomFieldID, "all-null geometry
column must not record an upper bound")
+
+ // The non-geo id column is unaffected by the geometry column being all
null.
+ require.Contains(t, lower, geoTestIDFieldID, "non-geo id column must
still record a lower bound")
+ require.Contains(t, upper, geoTestIDFieldID, "non-geo id column must
still record an upper bound")
+}
+
+// TestWriteGeometryColumnMetricsNone sets
write.metadata.metrics.column.geom=none
+// and asserts computeStatsPlan actually suppresses the geometry bounds. This
+// exercises the stats-plan dispatch (arrowStatsCollector -> applyGeoBounds)
that
+// internal.TestWriteDataFileGeoBounds cannot reach, since that test feeds
+// hand-built StatsCols straight to WriteDataFile. The adjacent non-geo id
column
+// keeps its ordinary bounds so the suppression is scoped to the geometry
column.
+func TestWriteGeometryColumnMetricsNone(t *testing.T) {
+ t.Parallel()
+
+ props := iceberg.Properties{
+ MetricsModeColumnConfPrefix + ".geom":
string(tblutils.MetricModeNone),
+ }
+ writer, schema, arrowSchema := newGeoTestWriter(t, t.TempDir(), props)
+
+ geomLo := wktToWKB(t, "POINT (0 -5)")
+ geomHi := wktToWKB(t, "POINT (30 10)")
+
+ rec, _, err := array.RecordFromJSON(memory.DefaultAllocator,
arrowSchema, strings.NewReader(`[
+ {"id": 1, "geom": "`+geomLo.String()+`", "geog": null},
+ {"id": 2, "geom": "`+geomHi.String()+`", "geog": null}
+ ]`))
+ require.NoError(t, err)
+ defer rec.Release()
+
+ df, err := writer.writeFile(t.Context(), nil, WriteTask{
+ Uuid: uuid.New(),
+ ID: 0,
+ FileCount: 1,
+ Schema: schema,
+ Batches: []arrow.RecordBatch{rec},
+ })
+ require.NoError(t, err)
+ require.EqualValues(t, 2, df.Count())
+
+ lower := df.LowerBoundValues()
+ upper := df.UpperBoundValues()
+ assert.NotContains(t, lower, geoTestGeomFieldID, "geometry column
bounds must be suppressed under metrics mode none")
+ assert.NotContains(t, upper, geoTestGeomFieldID, "geometry column
bounds must be suppressed under metrics mode none")
+
+ // Suppression is scoped to the geom column: id still gets ordinary
bounds.
+ require.Contains(t, lower, geoTestIDFieldID, "non-geo id column must
keep bounds when only geom is set to none")
+ require.Contains(t, upper, geoTestIDFieldID, "non-geo id column must
keep bounds when only geom is set to none")
+}
+
+// TestWriteGeometryColumnCheckedAllocator runs the geometry write path on a
+// checked allocator and asserts zero residual once the writer is done,
catching
+// a silent allocation escape in the geo-bounds accumulator wiring. This
mirrors
+// the memory.NewCheckedAllocator/AssertSize pattern the writer suites use.
+func TestWriteGeometryColumnCheckedAllocator(t *testing.T) {
+ t.Parallel()
+
+ mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
+ defer mem.AssertSize(t, 0)
+ ctx := compute.WithAllocator(t.Context(), mem)
+
+ writer, schema, arrowSchema := newGeoTestWriter(t, t.TempDir(),
iceberg.Properties{})
+
+ geomLo := wktToWKB(t, "POINT (0 -5)")
+ geomHi := wktToWKB(t, "POINT (30 10)")
+ geog := wktToWKB(t, "POINT (12 4)")
+
+ rec, _, err := array.RecordFromJSON(mem, arrowSchema,
strings.NewReader(`[
+ {"id": 1, "geom": "`+geomLo.String()+`", "geog":
"`+geog.String()+`"},
+ {"id": 2, "geom": "`+geomHi.String()+`", "geog": null}
+ ]`))
+ require.NoError(t, err)
+ defer rec.Release()
+
+ df, err := writer.writeFile(ctx, nil, WriteTask{
+ Uuid: uuid.New(),
+ ID: 0,
+ FileCount: 1,
+ Schema: schema,
+ Batches: []arrow.RecordBatch{rec},
+ })
+ require.NoError(t, err)
+ require.EqualValues(t, 2, df.Count())
+ require.Contains(t, df.LowerBoundValues(), geoTestGeomFieldID,
"geometry column must record a lower bound")
Review Comment:
the other three tests check both lower and upper bounds; here we only assert
the lower. I'd add the matching `df.UpperBoundValues()` contains check so this
one lines up and pins both ends.
--
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]
