You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "tanmayrauth (via GitHub)" <gi...@apache.org> on 2026/07/04 01:28:42 UTC
[PR] feat(table): emit geo bounds in manifest DataFile [iceberg-go]
tanmayrauth opened a new pull request, #1371:
URL: https://github.com/apache/iceberg-go/pull/1371
Compute geometry/geography bounding boxes from WKB during writes and emit WKB single-point lower/upper bounds into the manifest DataFile, which is what Iceberg pruning consumes. arrow-go cannot emit Parquet column-chunk GeospatialStatistics, so bounds are computed in iceberg-go.
Also fixes a panic when writing geo columns through the stats path, and lets geo bounds round-trip via LiteralFromBytes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): emit geo bounds in manifest DataFile [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on PR #1371:
URL: https://github.com/apache/iceberg-go/pull/1371#issuecomment-4927814108
@tanmayrauth give me couple more moments, almost done
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): emit geo bounds in manifest DataFile [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on code in PR #1371:
URL: https://github.com/apache/iceberg-go/pull/1371#discussion_r3540114537
##########
table/internal/geo_codec.go:
##########
@@ -0,0 +1,235 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package internal
+
+import (
+ "encoding/binary"
+ "math"
+
+ "github.com/apache/iceberg-go"
+ "github.com/twpayne/go-geom"
+ "github.com/twpayne/go-geom/encoding/wkb"
+)
+
+// Geo bounding box dimension indices. X is longitude/easting, Y is
+// latitude/northing; Z and M are optional. These index the per-dimension
+// min/max/has arrays of geoBoundsAccumulator.
+const (
+ geoDimX = iota
+ geoDimY
+ geoDimZ
+ geoDimM
+ geoNumDims
+)
+
+// geoBoundsAccumulator computes a geospatial bounding box from a stream of WKB
+// values. Iceberg stores geometry/geography column bounds using the single-value
+// serialization for geospatial types (spec Appendix D): the concatenation of
+// little-endian float64 coordinate values in X, Y[, Z][, M] order. The lower
+// bound carries the per-dimension minimums and the upper bound the maximums.
+// Null and NaN coordinate values are skipped, and a dimension that never sees a
+// finite value is omitted from the resulting bounds. If the X or Y dimension is
+// missing entirely, no bounding box is produced.
+//
+// Bounds are only emitted for geometry (planar edges). Geography bounds are
+// omitted: see Bounds.
+type geoBoundsAccumulator struct {
+ min [geoNumDims]float64
+ max [geoNumDims]float64
+ has [geoNumDims]bool
+
+ // isGeography marks a column whose edges are geodesics on a sphere, for
+ // which raw vertex min/max is not a safe bounding box (see Bounds).
+ isGeography bool
+}
+
+func newGeoBoundsAccumulator(isGeography bool) *geoBoundsAccumulator {
+ return &geoBoundsAccumulator{isGeography: isGeography}
+}
+
+// AddWKB unmarshals a single WKB value and extends the bounding box with its
+// coordinates.
+func (a *geoBoundsAccumulator) AddWKB(data []byte) error {
+ g, err := wkb.Unmarshal(data)
+ if err != nil {
+ return err
+ }
+
+ a.extend(g)
+
+ return nil
+}
+
+// extend walks every coordinate of g, recursing into geometry collections,
+// which cannot expose flat coordinates directly.
+func (a *geoBoundsAccumulator) extend(g geom.T) {
+ if gc, ok := g.(*geom.GeometryCollection); ok {
+ for _, sub := range gc.Geoms() {
+ a.extend(sub)
+ }
+
+ return
+ }
+
+ stride := g.Stride()
+ flat := g.FlatCoords()
+ if stride < 2 || len(flat) == 0 {
+ return
+ }
+
+ layout := g.Layout()
+ zIdx, mIdx := layout.ZIndex(), layout.MIndex()
+ for base := 0; base+stride <= len(flat); base += stride {
+ a.update(geoDimX, flat[base])
+ a.update(geoDimY, flat[base+1])
+ if zIdx >= 0 {
+ a.update(geoDimZ, flat[base+zIdx])
+ }
+ if mIdx >= 0 {
+ a.update(geoDimM, flat[base+mIdx])
+ }
+ }
+}
+
+func (a *geoBoundsAccumulator) update(dim int, v float64) {
+ if math.IsNaN(v) {
+ return
+ }
+
+ if !a.has[dim] {
+ a.min[dim], a.max[dim] = v, v
+ a.has[dim] = true
+
+ return
+ }
+
+ if v < a.min[dim] {
+ a.min[dim] = v
+ }
+ if v > a.max[dim] {
+ a.max[dim] = v
+ }
+}
+
+// layout selects the point layout for the bound points from the dimensions that
+// saw finite values. The bool is false when the box has no X or Y dimension and
+// therefore should not be emitted.
+func (a *geoBoundsAccumulator) layout() (geom.Layout, bool) {
+ if !a.has[geoDimX] || !a.has[geoDimY] {
+ return geom.NoLayout, false
+ }
+
+ switch {
+ case a.has[geoDimZ] && a.has[geoDimM]:
Review Comment:
Good catch — this is a real wrong-answer bug. I went slightly broader than the Z/M co-occurrence flag, because tracking only "seen together" still leaves an identical hole: an XYZM + XYZ mix would keep M and emit bounds claiming the XYZ row has an M in range.
##########
table/internal/parquet_files.go:
##########
@@ -436,6 +524,44 @@ func (w *ParquetFileWriter) Abort() error {
return errors.Join(closeErr, removeErr)
}
+// applyGeoBounds injects the WKB single-point bounds accumulated during the
+// write into the file statistics, so they flow through ToDataFile into the
+// manifest entry like any other typed bound.
+func (w *ParquetFileWriter) applyGeoBounds(stats *DataFileStatistics) error {
+ for fieldID, acc := range w.geoAccs {
+ // Honor the column's metrics mode: counts/none modes do not record bounds.
+ switch w.info.StatsCols[fieldID].Mode.Typ {
Review Comment:
Added MetricModeNone and MetricModeCounts sub-cases to TestWriteDataFileGeoBounds asserting the geo field ID is absent from lower/upper — that's exactly the fallthrough you flagged.
##########
table/internal/geo_codec.go:
##########
@@ -0,0 +1,235 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package internal
+
+import (
+ "encoding/binary"
+ "math"
+
+ "github.com/apache/iceberg-go"
+ "github.com/twpayne/go-geom"
+ "github.com/twpayne/go-geom/encoding/wkb"
+)
+
+// Geo bounding box dimension indices. X is longitude/easting, Y is
+// latitude/northing; Z and M are optional. These index the per-dimension
+// min/max/has arrays of geoBoundsAccumulator.
+const (
+ geoDimX = iota
+ geoDimY
+ geoDimZ
+ geoDimM
+ geoNumDims
+)
+
+// geoBoundsAccumulator computes a geospatial bounding box from a stream of WKB
+// values. Iceberg stores geometry/geography column bounds using the single-value
+// serialization for geospatial types (spec Appendix D): the concatenation of
+// little-endian float64 coordinate values in X, Y[, Z][, M] order. The lower
+// bound carries the per-dimension minimums and the upper bound the maximums.
+// Null and NaN coordinate values are skipped, and a dimension that never sees a
+// finite value is omitted from the resulting bounds. If the X or Y dimension is
+// missing entirely, no bounding box is produced.
+//
+// Bounds are only emitted for geometry (planar edges). Geography bounds are
+// omitted: see Bounds.
+type geoBoundsAccumulator struct {
+ min [geoNumDims]float64
+ max [geoNumDims]float64
+ has [geoNumDims]bool
+
+ // isGeography marks a column whose edges are geodesics on a sphere, for
+ // which raw vertex min/max is not a safe bounding box (see Bounds).
+ isGeography bool
+}
+
+func newGeoBoundsAccumulator(isGeography bool) *geoBoundsAccumulator {
+ return &geoBoundsAccumulator{isGeography: isGeography}
+}
+
+// AddWKB unmarshals a single WKB value and extends the bounding box with its
+// coordinates.
+func (a *geoBoundsAccumulator) AddWKB(data []byte) error {
+ g, err := wkb.Unmarshal(data)
+ if err != nil {
+ return err
+ }
+
+ a.extend(g)
+
+ return nil
+}
+
+// extend walks every coordinate of g, recursing into geometry collections,
+// which cannot expose flat coordinates directly.
+func (a *geoBoundsAccumulator) extend(g geom.T) {
+ if gc, ok := g.(*geom.GeometryCollection); ok {
+ for _, sub := range gc.Geoms() {
+ a.extend(sub)
+ }
+
+ return
+ }
+
+ stride := g.Stride()
+ flat := g.FlatCoords()
+ if stride < 2 || len(flat) == 0 {
+ return
+ }
+
+ layout := g.Layout()
+ zIdx, mIdx := layout.ZIndex(), layout.MIndex()
+ for base := 0; base+stride <= len(flat); base += stride {
+ a.update(geoDimX, flat[base])
+ a.update(geoDimY, flat[base+1])
+ if zIdx >= 0 {
+ a.update(geoDimZ, flat[base+zIdx])
+ }
+ if mIdx >= 0 {
+ a.update(geoDimM, flat[base+mIdx])
+ }
+ }
+}
+
+func (a *geoBoundsAccumulator) update(dim int, v float64) {
+ if math.IsNaN(v) {
+ return
+ }
+
+ if !a.has[dim] {
+ a.min[dim], a.max[dim] = v, v
+ a.has[dim] = true
+
+ return
+ }
+
+ if v < a.min[dim] {
+ a.min[dim] = v
+ }
+ if v > a.max[dim] {
+ a.max[dim] = v
+ }
+}
+
+// layout selects the point layout for the bound points from the dimensions that
+// saw finite values. The bool is false when the box has no X or Y dimension and
+// therefore should not be emitted.
+func (a *geoBoundsAccumulator) layout() (geom.Layout, bool) {
+ if !a.has[geoDimX] || !a.has[geoDimY] {
+ return geom.NoLayout, false
+ }
+
+ switch {
+ case a.has[geoDimZ] && a.has[geoDimM]:
+ return geom.XYZM, true
+ case a.has[geoDimZ]:
+ return geom.XYZ, true
+ case a.has[geoDimM]:
+ return geom.XYM, true
+ default:
+ return geom.XY, true
+ }
+}
+
+// encodeGeoBound serializes a bound point using the Iceberg single-value
+// serialization for geospatial types: little-endian float64 coordinates in
+// X, Y[, Z][, M] order. Lengths are XY=16, XYZ=24, XYM=32, XYZM=32 bytes. For
+// XYM the Z slot is written as NaN so a reader can tell XYM (NaN in slot 3)
+// apart from XYZM (finite Z in slot 3).
+func encodeGeoBound(vals [geoNumDims]float64, layout geom.Layout) []byte {
+ var coords []float64
+ switch layout {
+ case geom.XYZ:
+ coords = []float64{vals[geoDimX], vals[geoDimY], vals[geoDimZ]}
+ case geom.XYM:
+ coords = []float64{vals[geoDimX], vals[geoDimY], math.NaN(), vals[geoDimM]}
+ case geom.XYZM:
+ coords = []float64{vals[geoDimX], vals[geoDimY], vals[geoDimZ], vals[geoDimM]}
+ default:
+ coords = []float64{vals[geoDimX], vals[geoDimY]}
+ }
+
+ buf := make([]byte, len(coords)*8)
+ for i, c := range coords {
+ binary.LittleEndian.PutUint64(buf[i*8:], math.Float64bits(c))
+ }
+
+ return buf
+}
+
+// Bounds encodes the lower and upper bound points using the Iceberg geospatial
+// single-value serialization (see encodeGeoBound). Both are nil when no bounding
+// box could be produced (e.g. an all-null column).
+//
+// Geography bounds are always omitted. Geography edges are geodesics on a
+// sphere, so a box built from raw vertex min/max is unsafe: a geodesic edge can
+// reach a higher latitude than either endpoint, and the correct longitude
+// interval may wrap across the antimeridian (xmin > xmax). Emitting naive vertex
+// bounds would prune rows that actually fall inside a query region — a
+// silent-wrong-results bug. Missing bounds only disable pruning, which is always
+// safe, so geography is left unbounded until geodesic/antimeridian-aware
+// computation is added.
+func (a *geoBoundsAccumulator) Bounds() (lower, upper []byte) {
+ layout, ok := a.layout()
+ if !ok || a.isGeography {
+ return nil, nil
+ }
+
+ return encodeGeoBound(a.min, layout), encodeGeoBound(a.max, layout)
+}
+
+// StatsAgg materializes the accumulated bounds into a StatsAgg carrying the
+// precomputed single-point bounds, or nil when no box was produced. Unlike the
+// generic statsAggregator, geo bounds are computed from raw WKB during the write
+// (Parquet byte-array min/max over WKB are meaningless), so the aggregator just
+// replays the precomputed bytes through the existing ToDataFile bounds plumbing.
+func (a *geoBoundsAccumulator) StatsAgg() (StatsAgg, error) {
+ lower, upper := a.Bounds()
+ if lower == nil {
+ return nil, nil
+ }
+
+ return &geoStatsAgg{lower: lower, upper: upper}, nil
+}
+
+// geoStatsAgg is a StatsAgg holding precomputed geo single-point bounds encoded
+// with the Iceberg geospatial single-value serialization (see encodeGeoBound).
+type geoStatsAgg struct {
+ lower, upper []byte
+}
+
+func (g *geoStatsAgg) Min() iceberg.Literal {
+ if g.lower == nil {
+ return nil
+ }
+
+ return iceberg.BinaryLiteral(g.lower)
+}
+
+func (g *geoStatsAgg) Max() iceberg.Literal {
+ if g.upper == nil {
+ return nil
+ }
+
+ return iceberg.BinaryLiteral(g.upper)
+}
+
+func (g *geoStatsAgg) Update(interface{ HasMinMax() bool }) {}
Review Comment:
Documented the contract explicitly rather than restructuring: geoStatsAgg carries bounds precomputed from raw WKB, is injected into ColAggs only after DataFileStatsFromMeta has finished folding Parquet stats, and is drained via MinAsBytes/MaxAsBytes in ToDataFile — so nothing calls Update, and a call would discard precomputed bounds. I kept it on the StatsAgg interface since a separate carrier means a parallel path through DataFileStatistics/ToDataFile, which felt heavier than the risk warranted. Happy to switch to the explicit carrier if you'd still prefer it.
##########
table/internal/geo_codec.go:
##########
@@ -0,0 +1,235 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package internal
+
+import (
+ "encoding/binary"
+ "math"
+
+ "github.com/apache/iceberg-go"
+ "github.com/twpayne/go-geom"
+ "github.com/twpayne/go-geom/encoding/wkb"
+)
+
+// Geo bounding box dimension indices. X is longitude/easting, Y is
+// latitude/northing; Z and M are optional. These index the per-dimension
+// min/max/has arrays of geoBoundsAccumulator.
+const (
+ geoDimX = iota
+ geoDimY
+ geoDimZ
+ geoDimM
+ geoNumDims
+)
+
+// geoBoundsAccumulator computes a geospatial bounding box from a stream of WKB
+// values. Iceberg stores geometry/geography column bounds using the single-value
+// serialization for geospatial types (spec Appendix D): the concatenation of
+// little-endian float64 coordinate values in X, Y[, Z][, M] order. The lower
+// bound carries the per-dimension minimums and the upper bound the maximums.
+// Null and NaN coordinate values are skipped, and a dimension that never sees a
+// finite value is omitted from the resulting bounds. If the X or Y dimension is
+// missing entirely, no bounding box is produced.
+//
+// Bounds are only emitted for geometry (planar edges). Geography bounds are
+// omitted: see Bounds.
+type geoBoundsAccumulator struct {
+ min [geoNumDims]float64
+ max [geoNumDims]float64
+ has [geoNumDims]bool
+
+ // isGeography marks a column whose edges are geodesics on a sphere, for
+ // which raw vertex min/max is not a safe bounding box (see Bounds).
+ isGeography bool
+}
+
+func newGeoBoundsAccumulator(isGeography bool) *geoBoundsAccumulator {
+ return &geoBoundsAccumulator{isGeography: isGeography}
+}
+
+// AddWKB unmarshals a single WKB value and extends the bounding box with its
+// coordinates.
+func (a *geoBoundsAccumulator) AddWKB(data []byte) error {
+ g, err := wkb.Unmarshal(data)
+ if err != nil {
+ return err
+ }
+
+ a.extend(g)
+
+ return nil
+}
+
+// extend walks every coordinate of g, recursing into geometry collections,
+// which cannot expose flat coordinates directly.
+func (a *geoBoundsAccumulator) extend(g geom.T) {
+ if gc, ok := g.(*geom.GeometryCollection); ok {
+ for _, sub := range gc.Geoms() {
+ a.extend(sub)
+ }
+
+ return
+ }
+
+ stride := g.Stride()
+ flat := g.FlatCoords()
+ if stride < 2 || len(flat) == 0 {
+ return
+ }
+
+ layout := g.Layout()
+ zIdx, mIdx := layout.ZIndex(), layout.MIndex()
+ for base := 0; base+stride <= len(flat); base += stride {
+ a.update(geoDimX, flat[base])
+ a.update(geoDimY, flat[base+1])
+ if zIdx >= 0 {
+ a.update(geoDimZ, flat[base+zIdx])
+ }
+ if mIdx >= 0 {
+ a.update(geoDimM, flat[base+mIdx])
+ }
+ }
+}
+
+func (a *geoBoundsAccumulator) update(dim int, v float64) {
+ if math.IsNaN(v) {
+ return
Review Comment:
golangci-lint run is green on the current tip (0 issues), including the early returns in update()/extend() and the new returns in layout() — looks like this was already resolved. Let me know if CI still flags something and I'll chase the exact line.
##########
table/internal/parquet_files.go:
##########
@@ -384,14 +425,57 @@ func (p parquetFormat) NewFileWriter(ctx context.Context, fs iceio.WriteFileIO,
info: info,
partition: partitionValues,
colMapping: colMapping,
+ geoCols: geoCols,
+ geoAccs: geoAccs,
}, nil
}
// Write appends a record batch to the Parquet file.
func (w *ParquetFileWriter) Write(batch arrow.RecordBatch) error {
+ if err := w.accumulateGeoBounds(batch); err != nil {
+ return err
+ }
+
return w.pqWriter.WriteBuffered(batch)
}
+// wkbStorage is the subset of the binary Arrow arrays that back a geoarrow WKB
+// column (Binary and LargeBinary both satisfy it).
+type wkbStorage interface {
+ arrow.Array
+ Value(int) []byte
+}
+
+// accumulateGeoBounds extends the per-field bounding boxes with the WKB values
+// in this batch. Null rows are skipped; a malformed WKB value fails the write.
+func (w *ParquetFileWriter) accumulateGeoBounds(batch arrow.RecordBatch) error {
+ for _, gc := range w.geoCols {
+ if gc.colIdx >= int(batch.NumCols()) {
+ continue
+ }
+ ext, ok := batch.Column(gc.colIdx).(array.ExtensionArray)
+ if !ok {
+ continue
+ }
+ storage, ok := ext.Storage().(wkbStorage)
+ if !ok {
+ continue
+ }
+
+ acc := w.geoAccs[gc.fieldID]
Review Comment:
Done — made it a two-return + continue so a future change to how geoAccs is populated can't turn a missing key into a nil-panic on acc.AddWKB.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): emit geo bounds in manifest DataFile [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1371:
URL: https://github.com/apache/iceberg-go/pull/1371
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): emit geo bounds in manifest DataFile [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1371:
URL: https://github.com/apache/iceberg-go/pull/1371#discussion_r3539738534
##########
table/internal/parquet_files.go:
##########
@@ -436,6 +524,44 @@ func (w *ParquetFileWriter) Abort() error {
return errors.Join(closeErr, removeErr)
}
+// applyGeoBounds injects the WKB single-point bounds accumulated during the
+// write into the file statistics, so they flow through ToDataFile into the
+// manifest entry like any other typed bound.
+func (w *ParquetFileWriter) applyGeoBounds(stats *DataFileStatistics) error {
+ for fieldID, acc := range w.geoAccs {
+ // Honor the column's metrics mode: counts/none modes do not record bounds.
+ switch w.info.StatsCols[fieldID].Mode.Typ {
Review Comment:
`w.info.StatsCols[fieldID]` on a missing key returns a zero-value `StatisticsCollector` whose `Mode.Typ` is `""`, which matches neither `MetricModeNone` nor `MetricModeCounts` — so we fall through and write bounds for a column the caller never registered. That silently overrides an intent to skip.
I'd gate on presence first, and match the positive-check idiom the rest of this file already uses so the exhaustive linter stays happy:
```go
sc, ok := w.info.StatsCols[fieldID]
if !ok || sc.Mode.Typ == MetricModeNone || sc.Mode.Typ == MetricModeCounts {
continue
}
```
##########
table/internal/geo_codec.go:
##########
@@ -0,0 +1,235 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package internal
+
+import (
+ "encoding/binary"
+ "math"
+
+ "github.com/apache/iceberg-go"
+ "github.com/twpayne/go-geom"
+ "github.com/twpayne/go-geom/encoding/wkb"
+)
+
+// Geo bounding box dimension indices. X is longitude/easting, Y is
+// latitude/northing; Z and M are optional. These index the per-dimension
+// min/max/has arrays of geoBoundsAccumulator.
+const (
+ geoDimX = iota
+ geoDimY
+ geoDimZ
+ geoDimM
+ geoNumDims
+)
+
+// geoBoundsAccumulator computes a geospatial bounding box from a stream of WKB
+// values. Iceberg stores geometry/geography column bounds using the single-value
+// serialization for geospatial types (spec Appendix D): the concatenation of
+// little-endian float64 coordinate values in X, Y[, Z][, M] order. The lower
+// bound carries the per-dimension minimums and the upper bound the maximums.
+// Null and NaN coordinate values are skipped, and a dimension that never sees a
+// finite value is omitted from the resulting bounds. If the X or Y dimension is
+// missing entirely, no bounding box is produced.
+//
+// Bounds are only emitted for geometry (planar edges). Geography bounds are
+// omitted: see Bounds.
+type geoBoundsAccumulator struct {
+ min [geoNumDims]float64
+ max [geoNumDims]float64
+ has [geoNumDims]bool
+
+ // isGeography marks a column whose edges are geodesics on a sphere, for
+ // which raw vertex min/max is not a safe bounding box (see Bounds).
+ isGeography bool
+}
+
+func newGeoBoundsAccumulator(isGeography bool) *geoBoundsAccumulator {
+ return &geoBoundsAccumulator{isGeography: isGeography}
+}
+
+// AddWKB unmarshals a single WKB value and extends the bounding box with its
+// coordinates.
+func (a *geoBoundsAccumulator) AddWKB(data []byte) error {
+ g, err := wkb.Unmarshal(data)
+ if err != nil {
+ return err
+ }
+
+ a.extend(g)
+
+ return nil
+}
+
+// extend walks every coordinate of g, recursing into geometry collections,
+// which cannot expose flat coordinates directly.
+func (a *geoBoundsAccumulator) extend(g geom.T) {
+ if gc, ok := g.(*geom.GeometryCollection); ok {
+ for _, sub := range gc.Geoms() {
+ a.extend(sub)
+ }
+
+ return
+ }
+
+ stride := g.Stride()
+ flat := g.FlatCoords()
+ if stride < 2 || len(flat) == 0 {
+ return
+ }
+
+ layout := g.Layout()
+ zIdx, mIdx := layout.ZIndex(), layout.MIndex()
+ for base := 0; base+stride <= len(flat); base += stride {
+ a.update(geoDimX, flat[base])
+ a.update(geoDimY, flat[base+1])
+ if zIdx >= 0 {
+ a.update(geoDimZ, flat[base+zIdx])
+ }
+ if mIdx >= 0 {
+ a.update(geoDimM, flat[base+mIdx])
+ }
+ }
+}
+
+func (a *geoBoundsAccumulator) update(dim int, v float64) {
+ if math.IsNaN(v) {
+ return
+ }
+
+ if !a.has[dim] {
+ a.min[dim], a.max[dim] = v, v
+ a.has[dim] = true
+
+ return
+ }
+
+ if v < a.min[dim] {
+ a.min[dim] = v
+ }
+ if v > a.max[dim] {
+ a.max[dim] = v
+ }
+}
+
+// layout selects the point layout for the bound points from the dimensions that
+// saw finite values. The bool is false when the box has no X or Y dimension and
+// therefore should not be emitted.
+func (a *geoBoundsAccumulator) layout() (geom.Layout, bool) {
+ if !a.has[geoDimX] || !a.has[geoDimY] {
+ return geom.NoLayout, false
+ }
+
+ switch {
+ case a.has[geoDimZ] && a.has[geoDimM]:
Review Comment:
I think there's a real bug here for a column that mixes dimensionalities.
`has[]` is set the first time *any* geometry contributes a given dimension, so a column with some `POINT Z` rows and some `POINT M` rows sets both `has[geoDimZ]` and `has[geoDimM]` and promotes to XYZM — but the Z range came only from the XYZ subset and the M range only from the XYM subset. The emitted bounds then claim every row has a valid Z *and* a valid M in those ranges, which no single row does. A pruner filtering on M could drop rows that only carry Z (and vice versa), so this is wrong-answer pruning once the read side exists.
I'd track "seen Z and M together in one geometry" vs "seen either at all," and when they never co-occur, omit the ambiguous dimension rather than promoting to XYZM. wdyt on that as the rule — omit-on-ambiguity?
##########
table/internal/geo_codec.go:
##########
@@ -0,0 +1,235 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package internal
+
+import (
+ "encoding/binary"
+ "math"
+
+ "github.com/apache/iceberg-go"
+ "github.com/twpayne/go-geom"
+ "github.com/twpayne/go-geom/encoding/wkb"
+)
+
+// Geo bounding box dimension indices. X is longitude/easting, Y is
+// latitude/northing; Z and M are optional. These index the per-dimension
+// min/max/has arrays of geoBoundsAccumulator.
+const (
+ geoDimX = iota
+ geoDimY
+ geoDimZ
+ geoDimM
+ geoNumDims
+)
+
+// geoBoundsAccumulator computes a geospatial bounding box from a stream of WKB
+// values. Iceberg stores geometry/geography column bounds using the single-value
+// serialization for geospatial types (spec Appendix D): the concatenation of
+// little-endian float64 coordinate values in X, Y[, Z][, M] order. The lower
+// bound carries the per-dimension minimums and the upper bound the maximums.
+// Null and NaN coordinate values are skipped, and a dimension that never sees a
+// finite value is omitted from the resulting bounds. If the X or Y dimension is
+// missing entirely, no bounding box is produced.
+//
+// Bounds are only emitted for geometry (planar edges). Geography bounds are
+// omitted: see Bounds.
+type geoBoundsAccumulator struct {
+ min [geoNumDims]float64
+ max [geoNumDims]float64
+ has [geoNumDims]bool
+
+ // isGeography marks a column whose edges are geodesics on a sphere, for
+ // which raw vertex min/max is not a safe bounding box (see Bounds).
+ isGeography bool
+}
+
+func newGeoBoundsAccumulator(isGeography bool) *geoBoundsAccumulator {
+ return &geoBoundsAccumulator{isGeography: isGeography}
+}
+
+// AddWKB unmarshals a single WKB value and extends the bounding box with its
+// coordinates.
+func (a *geoBoundsAccumulator) AddWKB(data []byte) error {
+ g, err := wkb.Unmarshal(data)
+ if err != nil {
+ return err
+ }
+
+ a.extend(g)
+
+ return nil
+}
+
+// extend walks every coordinate of g, recursing into geometry collections,
+// which cannot expose flat coordinates directly.
+func (a *geoBoundsAccumulator) extend(g geom.T) {
+ if gc, ok := g.(*geom.GeometryCollection); ok {
+ for _, sub := range gc.Geoms() {
+ a.extend(sub)
+ }
+
+ return
+ }
+
+ stride := g.Stride()
+ flat := g.FlatCoords()
+ if stride < 2 || len(flat) == 0 {
+ return
+ }
+
+ layout := g.Layout()
+ zIdx, mIdx := layout.ZIndex(), layout.MIndex()
+ for base := 0; base+stride <= len(flat); base += stride {
+ a.update(geoDimX, flat[base])
+ a.update(geoDimY, flat[base+1])
+ if zIdx >= 0 {
+ a.update(geoDimZ, flat[base+zIdx])
+ }
+ if mIdx >= 0 {
+ a.update(geoDimM, flat[base+mIdx])
+ }
+ }
+}
+
+func (a *geoBoundsAccumulator) update(dim int, v float64) {
+ if math.IsNaN(v) {
+ return
+ }
+
+ if !a.has[dim] {
+ a.min[dim], a.max[dim] = v, v
+ a.has[dim] = true
+
+ return
+ }
+
+ if v < a.min[dim] {
+ a.min[dim] = v
+ }
+ if v > a.max[dim] {
+ a.max[dim] = v
+ }
+}
+
+// layout selects the point layout for the bound points from the dimensions that
+// saw finite values. The bool is false when the box has no X or Y dimension and
+// therefore should not be emitted.
+func (a *geoBoundsAccumulator) layout() (geom.Layout, bool) {
+ if !a.has[geoDimX] || !a.has[geoDimY] {
+ return geom.NoLayout, false
+ }
+
+ switch {
+ case a.has[geoDimZ] && a.has[geoDimM]:
+ return geom.XYZM, true
+ case a.has[geoDimZ]:
+ return geom.XYZ, true
+ case a.has[geoDimM]:
+ return geom.XYM, true
+ default:
+ return geom.XY, true
+ }
+}
+
+// encodeGeoBound serializes a bound point using the Iceberg single-value
+// serialization for geospatial types: little-endian float64 coordinates in
+// X, Y[, Z][, M] order. Lengths are XY=16, XYZ=24, XYM=32, XYZM=32 bytes. For
+// XYM the Z slot is written as NaN so a reader can tell XYM (NaN in slot 3)
+// apart from XYZM (finite Z in slot 3).
+func encodeGeoBound(vals [geoNumDims]float64, layout geom.Layout) []byte {
+ var coords []float64
+ switch layout {
+ case geom.XYZ:
+ coords = []float64{vals[geoDimX], vals[geoDimY], vals[geoDimZ]}
+ case geom.XYM:
+ coords = []float64{vals[geoDimX], vals[geoDimY], math.NaN(), vals[geoDimM]}
+ case geom.XYZM:
+ coords = []float64{vals[geoDimX], vals[geoDimY], vals[geoDimZ], vals[geoDimM]}
+ default:
+ coords = []float64{vals[geoDimX], vals[geoDimY]}
+ }
+
+ buf := make([]byte, len(coords)*8)
+ for i, c := range coords {
+ binary.LittleEndian.PutUint64(buf[i*8:], math.Float64bits(c))
+ }
+
+ return buf
+}
+
+// Bounds encodes the lower and upper bound points using the Iceberg geospatial
+// single-value serialization (see encodeGeoBound). Both are nil when no bounding
+// box could be produced (e.g. an all-null column).
+//
+// Geography bounds are always omitted. Geography edges are geodesics on a
+// sphere, so a box built from raw vertex min/max is unsafe: a geodesic edge can
+// reach a higher latitude than either endpoint, and the correct longitude
+// interval may wrap across the antimeridian (xmin > xmax). Emitting naive vertex
+// bounds would prune rows that actually fall inside a query region — a
+// silent-wrong-results bug. Missing bounds only disable pruning, which is always
+// safe, so geography is left unbounded until geodesic/antimeridian-aware
+// computation is added.
+func (a *geoBoundsAccumulator) Bounds() (lower, upper []byte) {
+ layout, ok := a.layout()
+ if !ok || a.isGeography {
+ return nil, nil
+ }
+
+ return encodeGeoBound(a.min, layout), encodeGeoBound(a.max, layout)
+}
+
+// StatsAgg materializes the accumulated bounds into a StatsAgg carrying the
+// precomputed single-point bounds, or nil when no box was produced. Unlike the
+// generic statsAggregator, geo bounds are computed from raw WKB during the write
+// (Parquet byte-array min/max over WKB are meaningless), so the aggregator just
+// replays the precomputed bytes through the existing ToDataFile bounds plumbing.
+func (a *geoBoundsAccumulator) StatsAgg() (StatsAgg, error) {
+ lower, upper := a.Bounds()
+ if lower == nil {
+ return nil, nil
+ }
+
+ return &geoStatsAgg{lower: lower, upper: upper}, nil
+}
+
+// geoStatsAgg is a StatsAgg holding precomputed geo single-point bounds encoded
+// with the Iceberg geospatial single-value serialization (see encodeGeoBound).
+type geoStatsAgg struct {
+ lower, upper []byte
+}
+
+func (g *geoStatsAgg) Min() iceberg.Literal {
+ if g.lower == nil {
+ return nil
+ }
+
+ return iceberg.BinaryLiteral(g.lower)
+}
+
+func (g *geoStatsAgg) Max() iceberg.Literal {
+ if g.upper == nil {
+ return nil
+ }
+
+ return iceberg.BinaryLiteral(g.upper)
+}
+
+func (g *geoStatsAgg) Update(interface{ HasMinMax() bool }) {}
Review Comment:
This no-op is safe only because of injection ordering — nothing currently iterates `ColAggs` and calls `Update` on a geo agg. But that's an implicit invariant, and the first call site that does iterate will silently drop updates on geo fields.
At minimum I'd document why it's a no-op. Better would be to not pretend to be a live `StatsAgg` at all — a separate precomputed-bounds carrier (exposing just `MinAsBytes`/`MaxAsBytes`) drained on its own in `ToDataFile` would make the "these are precomputed, don't Update them" contract explicit instead of load-bearing on ordering. Either's fine, but I'd make it explicit.
##########
table/internal/parquet_files_test.go:
##########
@@ -1348,3 +1349,87 @@ func TestShreddedVariantReadRoundTrip(t *testing.T) {
assert.EqualValues(t, 2, obj.NumElements(), "row %d should have a + city", i)
}
}
+
+// TestWriteDataFileGeoBounds writes geometry and geography columns through the
+// full ParquetFileWriter path and asserts that the resulting DataFile carries
+// geometry single-point bounds (Iceberg geospatial single-value serialization)
+// in the manifest entry, while geography bounds are omitted as unsafe.
+func TestWriteDataFileGeoBounds(t *testing.T) {
Review Comment:
Two gates go untested right now. `TestWriteDataFileGeoBounds` runs `MetricModeFull` end-to-end, so it happens to cover the old panic path — but its stated intent is bounds correctness, and a future maintainer won't read it as the regression guard for the `createStatsAgg` panic. A one-line comment anchoring it to that issue (or a dedicated tiny test) would keep the guard from silently rotting.
Separately, I'd add sub-cases for `MetricModeNone` and `MetricModeCounts` asserting the geo field ID does *not* appear in lower/upper — that's exactly the fallthrough I flagged in `applyGeoBounds`, and a test there would have caught it.
##########
table/internal/geo_codec.go:
##########
@@ -0,0 +1,235 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package internal
+
+import (
+ "encoding/binary"
+ "math"
+
+ "github.com/apache/iceberg-go"
+ "github.com/twpayne/go-geom"
+ "github.com/twpayne/go-geom/encoding/wkb"
+)
+
+// Geo bounding box dimension indices. X is longitude/easting, Y is
+// latitude/northing; Z and M are optional. These index the per-dimension
+// min/max/has arrays of geoBoundsAccumulator.
+const (
+ geoDimX = iota
+ geoDimY
+ geoDimZ
+ geoDimM
+ geoNumDims
+)
+
+// geoBoundsAccumulator computes a geospatial bounding box from a stream of WKB
+// values. Iceberg stores geometry/geography column bounds using the single-value
+// serialization for geospatial types (spec Appendix D): the concatenation of
+// little-endian float64 coordinate values in X, Y[, Z][, M] order. The lower
+// bound carries the per-dimension minimums and the upper bound the maximums.
+// Null and NaN coordinate values are skipped, and a dimension that never sees a
+// finite value is omitted from the resulting bounds. If the X or Y dimension is
+// missing entirely, no bounding box is produced.
+//
+// Bounds are only emitted for geometry (planar edges). Geography bounds are
+// omitted: see Bounds.
+type geoBoundsAccumulator struct {
+ min [geoNumDims]float64
+ max [geoNumDims]float64
+ has [geoNumDims]bool
+
+ // isGeography marks a column whose edges are geodesics on a sphere, for
+ // which raw vertex min/max is not a safe bounding box (see Bounds).
+ isGeography bool
+}
+
+func newGeoBoundsAccumulator(isGeography bool) *geoBoundsAccumulator {
+ return &geoBoundsAccumulator{isGeography: isGeography}
+}
+
+// AddWKB unmarshals a single WKB value and extends the bounding box with its
+// coordinates.
+func (a *geoBoundsAccumulator) AddWKB(data []byte) error {
+ g, err := wkb.Unmarshal(data)
+ if err != nil {
+ return err
+ }
+
+ a.extend(g)
+
+ return nil
+}
+
+// extend walks every coordinate of g, recursing into geometry collections,
+// which cannot expose flat coordinates directly.
+func (a *geoBoundsAccumulator) extend(g geom.T) {
+ if gc, ok := g.(*geom.GeometryCollection); ok {
+ for _, sub := range gc.Geoms() {
+ a.extend(sub)
+ }
+
+ return
+ }
+
+ stride := g.Stride()
+ flat := g.FlatCoords()
+ if stride < 2 || len(flat) == 0 {
+ return
+ }
+
+ layout := g.Layout()
+ zIdx, mIdx := layout.ZIndex(), layout.MIndex()
+ for base := 0; base+stride <= len(flat); base += stride {
+ a.update(geoDimX, flat[base])
+ a.update(geoDimY, flat[base+1])
+ if zIdx >= 0 {
+ a.update(geoDimZ, flat[base+zIdx])
+ }
+ if mIdx >= 0 {
+ a.update(geoDimM, flat[base+mIdx])
+ }
+ }
+}
+
+func (a *geoBoundsAccumulator) update(dim int, v float64) {
+ if math.IsNaN(v) {
+ return
Review Comment:
`nlreturn` wants a blank line before this early `return`, and there's a second one in `extend()` after the `GeometryCollection` recursion. golangci-lint will fail the build on both — worth a quick pass so CI goes green.
##########
table/internal/parquet_files.go:
##########
@@ -384,14 +425,57 @@ func (p parquetFormat) NewFileWriter(ctx context.Context, fs iceio.WriteFileIO,
info: info,
partition: partitionValues,
colMapping: colMapping,
+ geoCols: geoCols,
+ geoAccs: geoAccs,
}, nil
}
// Write appends a record batch to the Parquet file.
func (w *ParquetFileWriter) Write(batch arrow.RecordBatch) error {
+ if err := w.accumulateGeoBounds(batch); err != nil {
+ return err
+ }
+
return w.pqWriter.WriteBuffered(batch)
}
+// wkbStorage is the subset of the binary Arrow arrays that back a geoarrow WKB
+// column (Binary and LargeBinary both satisfy it).
+type wkbStorage interface {
+ arrow.Array
+ Value(int) []byte
+}
+
+// accumulateGeoBounds extends the per-field bounding boxes with the WKB values
+// in this batch. Null rows are skipped; a malformed WKB value fails the write.
+func (w *ParquetFileWriter) accumulateGeoBounds(batch arrow.RecordBatch) error {
+ for _, gc := range w.geoCols {
+ if gc.colIdx >= int(batch.NumCols()) {
+ continue
+ }
+ ext, ok := batch.Column(gc.colIdx).(array.ExtensionArray)
+ if !ok {
+ continue
+ }
+ storage, ok := ext.Storage().(wkbStorage)
+ if !ok {
+ continue
+ }
+
+ acc := w.geoAccs[gc.fieldID]
Review Comment:
`acc` is safe by construction today, but a missing key here nil-panics on `acc.AddWKB`. Cheap to make it a two-return + `continue` so a future refactor that changes how `geoAccs` is populated can't turn this into a crash.
##########
table/internal/parquet_files.go:
##########
@@ -343,6 +344,34 @@ type ParquetFileWriter struct {
info WriteFileInfo
partition map[int]any
colMapping map[string]int
+ geoCols []geoColumn
+ geoAccs map[int]*geoBoundsAccumulator
+}
+
+// geoColumn locates a top-level geometry/geography column: its position in the
+// Arrow record batch and the Iceberg field ID its bounds are recorded under.
+type geoColumn struct {
+ colIdx int
+ fieldID int
+}
+
+// collectGeoColumns finds top-level WKB-encoded geo columns in the Arrow schema
+// and pairs each with its Iceberg field ID. Geo bounds for columns nested inside
+// structs/lists/maps are not yet computed.
+func collectGeoColumns(sc *arrow.Schema, colMapping map[string]int) []geoColumn {
+ var result []geoColumn
+ for i, f := range sc.Fields() {
Review Comment:
`collectGeoColumns` only walks top-level fields, so a geo column nested in a struct/list/map gets no bounds — silently, and diverging from Java/PyIceberg. That's a fine scope cut for this PR, but I'd file a tracking issue and drop a `// TODO(#issue): nested geo columns` here so it's discoverable rather than invisible.
##########
literals.go:
##########
@@ -174,6 +174,22 @@ func LiteralFromBytes(typ Type, data []byte) (Literal, error) {
var v BinaryLiteral
err := v.UnmarshalBinary(data)
+ return v, err
+ case GeometryType, GeographyType:
+ // Geometry/Geography single-value bounds use the Iceberg geospatial
+ // serialization (spec Appendix D): little-endian float64 coordinates in
+ // X, Y[, Z][, M] order, i.e. 16, 24, or 32 bytes — not WKB. iceberg-go
+ // has no geo literal for predicate evaluation, so the validated
+ // coordinate bytes are returned as an opaque BinaryLiteral.
+ switch len(data) {
+ case 16, 24, 32:
Review Comment:
Two small things. The empty `case 16, 24, 32:` reads like a forgotten body — a `// valid sizes, fall through` comment (or restructuring to a positive length check like `FixedType` above) makes the intent obvious.
And the geo case returns a plain `BinaryLiteral`, indistinguishable from a real binary bound. No bug today since nothing does geo pruning, but a future geo predicate evaluator calling `LiteralFromBytes` would get raw coordinate bytes with no signal they're geo. A short comment warning callers to check the original `typ` (or a thin wrapper type later) would save someone a debugging session.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): emit geo bounds in manifest DataFile [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1371:
URL: https://github.com/apache/iceberg-go/pull/1371#discussion_r3523576691
##########
table/internal/geo_codec.go:
##########
@@ -0,0 +1,215 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package internal
+
+import (
+ "math"
+
+ "github.com/apache/iceberg-go"
+ "github.com/twpayne/go-geom"
+ "github.com/twpayne/go-geom/encoding/wkb"
+)
+
+// Geo bounding box dimension indices. X is longitude/easting, Y is
+// latitude/northing; Z and M are optional. These index the per-dimension
+// min/max/has arrays of geoBoundsAccumulator.
+const (
+ geoDimX = iota
+ geoDimY
+ geoDimZ
+ geoDimM
+ geoNumDims
+)
+
+// geoBoundsAccumulator computes a geospatial bounding box from a stream of WKB
+// values. Iceberg stores geometry/geography column bounds as the WKB encoding
+// of two points: the lower bound carries the per-dimension minimums and the
+// upper bound carries the maximums. Per the Parquet/Iceberg geospatial spec,
+// null and NaN coordinate values are skipped, and a dimension that never sees a
+// finite value is omitted from the resulting points. If the X or Y dimension is
+// missing entirely, no bounding box is produced.
+type geoBoundsAccumulator struct {
+ min [geoNumDims]float64
+ max [geoNumDims]float64
+ has [geoNumDims]bool
+}
+
+func newGeoBoundsAccumulator() *geoBoundsAccumulator {
+ return &geoBoundsAccumulator{}
+}
+
+// AddWKB unmarshals a single WKB value and extends the bounding box with its
+// coordinates.
+func (a *geoBoundsAccumulator) AddWKB(data []byte) error {
+ g, err := wkb.Unmarshal(data)
+ if err != nil {
+ return err
+ }
+
+ a.extend(g)
+
+ return nil
+}
+
+// extend walks every coordinate of g, recursing into geometry collections,
+// which cannot expose flat coordinates directly.
+func (a *geoBoundsAccumulator) extend(g geom.T) {
+ if gc, ok := g.(*geom.GeometryCollection); ok {
+ for _, sub := range gc.Geoms() {
+ a.extend(sub)
+ }
+
+ return
+ }
+
+ stride := g.Stride()
+ flat := g.FlatCoords()
+ if stride < 2 || len(flat) == 0 {
+ return
+ }
+
+ layout := g.Layout()
+ zIdx, mIdx := layout.ZIndex(), layout.MIndex()
+ for base := 0; base+stride <= len(flat); base += stride {
+ a.update(geoDimX, flat[base])
+ a.update(geoDimY, flat[base+1])
+ if zIdx >= 0 {
+ a.update(geoDimZ, flat[base+zIdx])
+ }
+ if mIdx >= 0 {
+ a.update(geoDimM, flat[base+mIdx])
+ }
+ }
+}
+
+func (a *geoBoundsAccumulator) update(dim int, v float64) {
+ if math.IsNaN(v) {
+ return
+ }
+
+ if !a.has[dim] {
+ a.min[dim], a.max[dim] = v, v
+ a.has[dim] = true
+
+ return
+ }
+
+ if v < a.min[dim] {
+ a.min[dim] = v
+ }
+ if v > a.max[dim] {
+ a.max[dim] = v
+ }
+}
+
+// layout selects the point layout for the bound points from the dimensions that
+// saw finite values. The bool is false when the box has no X or Y dimension and
+// therefore should not be emitted.
+func (a *geoBoundsAccumulator) layout() (geom.Layout, bool) {
+ if !a.has[geoDimX] || !a.has[geoDimY] {
+ return geom.NoLayout, false
+ }
+
+ switch {
+ case a.has[geoDimZ] && a.has[geoDimM]:
+ return geom.XYZM, true
+ case a.has[geoDimZ]:
+ return geom.XYZ, true
+ case a.has[geoDimM]:
+ return geom.XYM, true
+ default:
+ return geom.XY, true
+ }
+}
+
+func geoPointCoords(vals [geoNumDims]float64, layout geom.Layout) []float64 {
+ switch layout {
+ case geom.XYZ:
+ return []float64{vals[geoDimX], vals[geoDimY], vals[geoDimZ]}
+ case geom.XYM:
+ return []float64{vals[geoDimX], vals[geoDimY], vals[geoDimM]}
+ case geom.XYZM:
+ return []float64{vals[geoDimX], vals[geoDimY], vals[geoDimZ], vals[geoDimM]}
+ default:
+ return []float64{vals[geoDimX], vals[geoDimY]}
+ }
+}
+
+// Bounds encodes the lower and upper bound points as WKB. Both are nil when no
+// bounding box could be produced (e.g. an all-null column).
+func (a *geoBoundsAccumulator) Bounds() (lower, upper []byte, err error) {
+ layout, ok := a.layout()
+ if !ok {
+ return nil, nil, nil
+ }
+
+ lower, err = wkb.Marshal(geom.NewPointFlat(layout, geoPointCoords(a.min, layout)), wkb.NDR)
Review Comment:
This serializes the manifest lower bound as a WKB point, and line 166 does the same for the upper bound. Iceberg v3 geo single-value bounds are not WKB; they are concatenated little-endian float64 coordinates (XY=16 bytes, XYZ=24, XYM/XYZM=32 with XYM Z=NaN). Please encode/decode these coordinate bytes instead, update `LiteralFromBytes` so geo bounds are parsed as the spec format rather than opaque WKB, and assert exact byte lengths/decoded coordinates in the tests.
##########
table/internal/parquet_files.go:
##########
@@ -384,14 +419,57 @@ func (p parquetFormat) NewFileWriter(ctx context.Context, fs iceio.WriteFileIO,
info: info,
partition: partitionValues,
colMapping: colMapping,
+ geoCols: geoCols,
+ geoAccs: geoAccs,
}, nil
}
// Write appends a record batch to the Parquet file.
func (w *ParquetFileWriter) Write(batch arrow.RecordBatch) error {
+ if err := w.accumulateGeoBounds(batch); err != nil {
+ return err
+ }
+
return w.pqWriter.WriteBuffered(batch)
}
+// wkbStorage is the subset of the binary Arrow arrays that back a geoarrow WKB
+// column (Binary and LargeBinary both satisfy it).
+type wkbStorage interface {
+ arrow.Array
+ Value(int) []byte
+}
+
+// accumulateGeoBounds extends the per-field bounding boxes with the WKB values
+// in this batch. Null rows are skipped; a malformed WKB value fails the write.
+func (w *ParquetFileWriter) accumulateGeoBounds(batch arrow.RecordBatch) error {
+ for _, gc := range w.geoCols {
+ if gc.colIdx >= int(batch.NumCols()) {
+ continue
+ }
+ ext, ok := batch.Column(gc.colIdx).(array.ExtensionArray)
+ if !ok {
+ continue
+ }
+ storage, ok := ext.Storage().(wkbStorage)
+ if !ok {
+ continue
+ }
+
+ acc := w.geoAccs[gc.fieldID]
+ for i := range storage.Len() {
+ if storage.IsNull(i) {
+ continue
+ }
+ if err := acc.AddWKB(storage.Value(i)); err != nil {
Review Comment:
The accumulator is fed raw WKB bytes without knowing whether the Iceberg field is geometry or geography, so geography bounds are computed by simple vertex min/max. That misses geodesic edge interpolation and antimeridian wrapping; for geography crossing the antimeridian the correct longitude interval can require `xmin > xmax`. Please make accumulation type-aware and compute safe geography bounds, or omit geography bounds when they cannot be computed safely, with antimeridian/geodesic coverage.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): emit geo bounds in manifest DataFile [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on PR #1371:
URL: https://github.com/apache/iceberg-go/pull/1371#issuecomment-4928159361
@laskoviymishka I've pushed the changes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): emit geo bounds in manifest DataFile [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on code in PR #1371:
URL: https://github.com/apache/iceberg-go/pull/1371#discussion_r3553697527
##########
literals.go:
##########
@@ -174,6 +174,24 @@ func LiteralFromBytes(typ Type, data []byte) (Literal, error) {
var v BinaryLiteral
err := v.UnmarshalBinary(data)
+ return v, err
+ case GeometryType, GeographyType:
Review Comment:
Good catch, agreed. I'll add a dedicated geo literal so .Type() reports Geometry/Geography honestly, and make its ordering paths (Comparator, To) error instead of silently running bytes.Compare, LE coord bytes have no valid ordering anyway. I'll also gate the ordering visitors in evaluators.go so a geo term returns rowsMightMatch rather than routing through the comparator. Pushing that now with a test. Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): emit geo bounds in manifest DataFile [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on code in PR #1371:
URL: https://github.com/apache/iceberg-go/pull/1371#discussion_r3540115284
##########
table/internal/parquet_files_test.go:
##########
@@ -1348,3 +1349,87 @@ func TestShreddedVariantReadRoundTrip(t *testing.T) {
assert.EqualValues(t, 2, obj.NumElements(), "row %d should have a + city", i)
}
}
+
+// TestWriteDataFileGeoBounds writes geometry and geography columns through the
+// full ParquetFileWriter path and asserts that the resulting DataFile carries
+// geometry single-point bounds (Iceberg geospatial single-value serialization)
+// in the manifest entry, while geography bounds are omitted as unsafe.
+func TestWriteDataFileGeoBounds(t *testing.T) {
Review Comment:
Added a comment stating the test guards the createStatsAgg panic via the isGeoType gate (geo types have no generic stats aggregator), and split the full-mode path into its own named sub-case so the intent reads clearly. The MetricModeNone/Counts sub-cases above cover the applyGeoBounds gating.
##########
literals.go:
##########
@@ -174,6 +174,22 @@ func LiteralFromBytes(typ Type, data []byte) (Literal, error) {
var v BinaryLiteral
err := v.UnmarshalBinary(data)
+ return v, err
+ case GeometryType, GeographyType:
+ // Geometry/Geography single-value bounds use the Iceberg geospatial
+ // serialization (spec Appendix D): little-endian float64 coordinates in
+ // X, Y[, Z][, M] order, i.e. 16, 24, or 32 bytes — not WKB. iceberg-go
+ // has no geo literal for predicate evaluation, so the validated
+ // coordinate bytes are returned as an opaque BinaryLiteral.
+ switch len(data) {
+ case 16, 24, 32:
Review Comment:
Both addressed: annotated case 16, 24, 32: as an intentional valid-sizes fall-through, and added a warning that the returned BinaryLiteral holds raw little-endian coordinates — a future geo predicate evaluator must check the original type rather than treating it as a comparable binary bound.
##########
table/internal/parquet_files.go:
##########
@@ -343,6 +344,34 @@ type ParquetFileWriter struct {
info WriteFileInfo
partition map[int]any
colMapping map[string]int
+ geoCols []geoColumn
+ geoAccs map[int]*geoBoundsAccumulator
+}
+
+// geoColumn locates a top-level geometry/geography column: its position in the
+// Arrow record batch and the Iceberg field ID its bounds are recorded under.
+type geoColumn struct {
+ colIdx int
+ fieldID int
+}
+
+// collectGeoColumns finds top-level WKB-encoded geo columns in the Arrow schema
+// and pairs each with its Iceberg field ID. Geo bounds for columns nested inside
+// structs/lists/maps are not yet computed.
+func collectGeoColumns(sc *arrow.Schema, colMapping map[string]int) []geoColumn {
+ var result []geoColumn
+ for i, f := range sc.Fields() {
Review Comment:
Added // TODO(#992): compute geo bounds for geo columns nested in structs/lists/maps and noted the divergence from Java/PyIceberg. I anchored it to #992 (this work's parent) for now — happy to file a dedicated follow-up issue and repoint the TODO if you'd prefer it tracked separately.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): emit geo bounds in manifest DataFile [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on PR #1371:
URL: https://github.com/apache/iceberg-go/pull/1371#issuecomment-4896932840
@zeroshade @laskoviymishka can you please review it?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): emit geo bounds in manifest DataFile [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on PR #1371:
URL: https://github.com/apache/iceberg-go/pull/1371#issuecomment-4927810779
@zeroshade I think we can merge this. I've incorporated all the requested changes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): emit geo bounds in manifest DataFile [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on PR #1371:
URL: https://github.com/apache/iceberg-go/pull/1371#issuecomment-4883301435
@zeroshade Thanks for both good catches. Switched manifest bounds to the spec coordinate encoding and made geography omit bounds for now.
On table/internal/geo_codec.go (the encodeGeoBound/Bounds area):
Fixed — bounds are now the Iceberg geospatial single-value serialization (little-endian float64 X, Y[, Z][, M]: 16/24/32 bytes, XYM writes Z=NaN), not WKB. `LiteralFromBytes` for geometry/geography now validates the 16/24/32 length instead of treating the payload as opaque WKB, and the tests assert exact byte lengths and the decoded coordinates (including the XYM NaN slot).
On table/internal/parquet_files.go (the AddWKB accumulation loop):
Made accumulation type-aware: the accumulator now knows geometry vs geography (from the Iceberg field type), and geography bounds are omitted entirely. Since geography edges are geodesics, vertex min/max isn't a safe box — latitude can bulge past the endpoints and longitude may need to wrap the antimeridian — so rather than emit possibly-wrong bounds that silently prune valid rows, I leave geography unbounded until we add geodesic/antimeridian-aware computation. Geometry (planar edges) still gets exact bounds.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): emit geo bounds in manifest DataFile [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1371:
URL: https://github.com/apache/iceberg-go/pull/1371#discussion_r3553605427
##########
literals.go:
##########
@@ -174,6 +174,24 @@ func LiteralFromBytes(typ Type, data []byte) (Literal, error) {
var v BinaryLiteral
err := v.UnmarshalBinary(data)
+ return v, err
+ case GeometryType, GeographyType:
Review Comment:
The validation and byte layout here are right, but the returned `BinaryLiteral` reports `.Type() == Binary`, not Geometry/Geography. We're safe today only because `evaluators.go` dispatches on `field.Type` rather than `lit.Type()` — the moment anything keys off the literal's own type (pruning, `.To()`, logging), it sees binary and routes through `bytes.Compare`, which isn't a valid ordering over LE coords and gives silently-wrong answers instead of erroring.
I'd rather not leave that as a doc comment asking every caller to check the source type. Could we add a thin `GeoLiteral` wrapper (or parametrize `BinaryLiteral`'s `Type()`) so `.Type()` is honest, and gate the ordering-comparison paths to reject geo terms explicitly? wdyt?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org