zeroshade commented on code in PR #1511:
URL: https://github.com/apache/iceberg-go/pull/1511#discussion_r3647331612
##########
table/internal/geo_codec.go:
##########
@@ -266,3 +274,166 @@ func (g *geoStatsAgg) Update(interface{ HasMinMax() bool
}) {}
func (g *geoStatsAgg) MinAsBytes() ([]byte, error) { return g.lower, nil }
func (g *geoStatsAgg) MaxAsBytes() ([]byte, error) { return g.upper, nil }
+
+// decodeGeoBound is the inverse of encodeGeoBound: it parses an Iceberg
+// geospatial single-value bound back into its per-dimension coordinates and
the
+// layout it was written with. A 32-byte bound is XYM when its Z slot is NaN
and
+// XYZM otherwise (see encodeGeoBound). ok is false when the length is not a
+// valid bound length (16, 24, or 32 bytes).
+func decodeGeoBound(data []byte) (vals [geoNumDims]float64, layout
geom.Layout, ok bool) {
+ var n int
+ switch len(data) {
+ case 16:
+ n = 2
+ case 24:
+ n = 3
+ case 32:
+ n = 4
+ default:
+ return vals, geom.NoLayout, false
+ }
+
+ coords := make([]float64, n)
+ for i := range coords {
+ coords[i] =
math.Float64frombits(binary.LittleEndian.Uint64(data[i*8:]))
+ }
+
+ vals[geoDimX], vals[geoDimY] = coords[0], coords[1]
+ switch len(data) {
+ case 16:
+ layout = geom.XY
+ case 24:
+ vals[geoDimZ], layout = coords[2], geom.XYZ
+ case 32:
+ if math.IsNaN(coords[2]) {
+ vals[geoDimM], layout = coords[3], geom.XYM
+ } else {
+ vals[geoDimZ], vals[geoDimM], layout = coords[2],
coords[3], geom.XYZM
+ }
+ }
+
+ return vals, layout, true
+}
+
+// layoutHasZM reports which optional dimensions a bound layout carries.
+func layoutHasZM(l geom.Layout) (hasZ, hasM bool) {
+ switch l {
+ case geom.XYZ:
+ return true, false
+ case geom.XYM:
+ return false, true
+ case geom.XYZM:
+ return true, true
+ default:
+ return false, false
+ }
+}
+
+// GeoBoundsAggregator combines the per-data-file geospatial bounds emitted by
+// geoBoundsAccumulator (the single-value serialization written into a
+// DataFile's lower/upper bounds; see Bounds) across multiple files into one
+// bounding box. It is the manifest-level analogue of the primitive min/max
+// aggregation the manifest writer already does per partition field, but geo
+// bounds are coordinate tuples with no total order, so they must never be
+// folded with a
+// scalar byte comparison: each bound is decoded to its coordinates and merged
+// dimension by dimension. The combined box is returned in the same
+// serialization, so it round-trips through a manifest bound exactly like a
+// per-file one.
+//
+// Geography columns emit no per-file bounds (see Bounds), so a geography
column
+// contributes nothing and the aggregate stays empty.
+type GeoBoundsAggregator struct {
+ min [geoNumDims]float64
+ max [geoNumDims]float64
+ has [geoNumDims]bool
+
+ // n counts the files added; zFiles and mFiles count how many carried a
Z / M
+ // dimension. An optional dimension is only emitted when every file
carried it
+ // (count == n) - the same omit-on-ambiguity rule geoBoundsAccumulator
applies
+ // across geometries within a single file. Emitting a Z/M that some
files lack
+ // would imply those files have a value in range and drive wrong-answer
+ // pruning.
+ n int
+ zFiles int
+ mFiles int
+}
+
+// Add merges one data file's lower and upper geo bounds into the aggregate.
The
+// two bounds must share a layout, which they always do: a file's lower and
upper
+// are written together by geoBoundsAccumulator. An empty pair (nil/empty lower
+// and upper) contributes nothing, so files without geo bounds - including
every
+// geography file - can be passed through harmlessly. It errors only when a
bound
+// is a non-empty but invalid length, or when lower and upper disagree on
layout.
+func (g *GeoBoundsAggregator) Add(lower, upper []byte) error {
+ if len(lower) == 0 && len(upper) == 0 {
+ return nil
+ }
+
+ lo, loLayout, ok := decodeGeoBound(lower)
+ if !ok {
+ return fmt.Errorf("%w: geo lower bound must be 16, 24, or 32
bytes, got %d",
+ iceberg.ErrInvalidBinSerialization, len(lower))
+ }
+ hi, hiLayout, ok := decodeGeoBound(upper)
+ if !ok {
+ return fmt.Errorf("%w: geo upper bound must be 16, 24, or 32
bytes, got %d",
+ iceberg.ErrInvalidBinSerialization, len(upper))
+ }
+ if loLayout != hiLayout {
+ return fmt.Errorf("%w: geo lower/upper bounds have mismatched
layouts (%v vs %v)",
+ iceberg.ErrInvalidBinSerialization, loLayout, hiLayout)
+ }
+
+ g.n++
+ g.update(geoDimX, lo[geoDimX], hi[geoDimX])
Review Comment:
`GeoBoundsAggregator.Add` always merges X with scalar min/max. Iceberg
geography allows `lower_x > upper_x` to denote an antimeridian-crossing box;
merging a wrapped file `[170,-170]` with `[10,20]` yields `[10,20]`, dropping
the wrapped range. This exported aggregator has no type input to distinguish
geometry vs geography — either refuse geography or implement interval-union for
longitude.
--
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]
