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: [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]
