laskoviymishka commented on code in PR #1597:
URL: https://github.com/apache/iceberg-go/pull/1597#discussion_r3673919799
##########
table/substrait/substrait.go:
##########
@@ -443,3 +443,11 @@ func (t *toSubstraitExpr) VisitNotStartsWith(term
iceberg.BoundTerm, lit iceberg
t.makeLitFunc(startsWithID, term, lit).(expr.FuncArgBuilder),
)
}
+
+func (t *toSubstraitExpr) VisitBBoxIntersects(iceberg.BoundTerm,
iceberg.BoundingBox) expr.Builder {
+ panic("geospatial bbox predicates cannot be converted to substrait")
Review Comment:
This panic isn't just defensive — it's reachable on what looks like the
primary use of the new API. Run `table.Scan(WithFilter(BBoxIntersects(...)))`
and the predicate survives column-name translation (because
`boundBBoxPredicate` satisfies `BoundUnaryPredicate`), survives `Bind`, and
lands here via `getRecordFilter` → `ConvertExpr`. `VisitExpr`'s `recover()`
turns it into an error, so the whole scan fails — which contradicts the
"conservatively keeps every row" contract the `exprEvaluator` advertises (that
path only runs for partition eval, never the record filter).
The real fix is upstream: drop the bbox predicate to `AlwaysTrue` before it
reaches substrait, either in `getRecordFilter` or by special-casing
`*boundBBoxPredicate` in `columnNameTranslator.VisitBound` (see my note there —
same fix closes the all-rows-dropped case too). Even as a backstop here, I'd
return `fmt.Errorf("%w: ...", ErrNotImplemented)` rather than a bare panic
string so callers get something typed and wrappable. wdyt?
##########
visitors.go:
##########
@@ -578,6 +604,13 @@ func (sanitizeVisitor) VisitBound(pred BoundPredicate)
BooleanExpression {
// literal serializes without needing the field's type.
ref := Reference(pred.Term().Ref().Field().Name)
switch p := pred.(type) {
+ case *boundBBoxPredicate:
Review Comment:
You guard this correctly here in `sanitizeVisitor`, but
`columnNameTranslator.VisitBound` has the same shape of switch and doesn't
special-case `*boundBBoxPredicate` — and its column-not-found early return
fires before the `BoundUnaryPredicate` arm is ever reached.
So a geo-filtered scan against a data file that predates the geometry column
hits the not-found path, returns `AlwaysFalse`, and silently drops every row
instead of keeping them. That's the one direction pruning must never be wrong.
The same guard you wrote here (match `*boundBBoxPredicate` → `AlwaysTrue`)
in `columnNameTranslator.VisitBound` fixes it, and because it also drops the
predicate before substrait, it resolves the panic path from my other comment in
one shot. More fundamentally, this is the cost of `boundBBoxPredicate`
deliberately satisfying `BoundUnaryPredicate` — every type-switch on that
interface now has to remember to match the concrete type first. Worth a hard
look at whether that trick is paying for itself. wdyt?
##########
table/internal/geo_codec.go:
##########
@@ -315,6 +315,34 @@ func decodeGeoBound(data []byte) (vals
[geoNumDims]float64, layout geom.Layout,
return vals, layout, true
}
+// GeoBoundsXY decodes a geometry column's lower and upper geo bounds (Iceberg
+// single-value serialization; see encodeGeoBound) into their planar XY
extents.
+// ok is false when either bound is missing, malformed, or carries a NaN X/Y
+// coordinate - all cases where the bound is unusable for pruning.
+func GeoBoundsXY(lower, upper []byte) (minX, minY, maxX, maxY float64, ok
bool) {
+ lo, _, okLo := decodeGeoBound(lower)
+ hi, _, okHi := decodeGeoBound(upper)
+ if !okLo || !okHi {
+ return 0, 0, 0, 0, false
+ }
+ if math.IsNaN(lo[geoDimX]) || math.IsNaN(lo[geoDimY]) ||
+ math.IsNaN(hi[geoDimX]) || math.IsNaN(hi[geoDimY]) {
+ return 0, 0, 0, 0, false
+ }
+
+ return lo[geoDimX], lo[geoDimY], hi[geoDimX], hi[geoDimY], true
Review Comment:
We reject NaN here but not inverted bounds. If a third-party writer emits
`lo[X] > hi[X]`, `BBoxIntersectsXY` gets `aMaxX < aMinX`, the intersection test
is always false, and we prune a file we should have kept — a dropped-rows false
negative, same wrong direction as the schema-evolution case.
iceberg-go's own accumulator never produces inverted bounds, but this is a
trust-on-read decode path, so the writer isn't ours to trust. I'd add `if
lo[geoDimX] > hi[geoDimX] || lo[geoDimY] > hi[geoDimY] { return 0, 0, 0, 0,
false }` right after the NaN check so a malformed bound just makes the file
unprunable. wdyt?
##########
visitors.go:
##########
@@ -60,6 +60,8 @@ type BoundBooleanExprVisitor[T any] interface {
VisitLess(BoundTerm, Literal) T
VisitStartsWith(BoundTerm, Literal) T
VisitNotStartsWith(BoundTerm, Literal) T
+ VisitBBoxIntersects(BoundTerm, BoundingBox) T
Review Comment:
Adding two methods to this exported interface breaks every external
implementation — Trino/Dremio adapters and downstream visitors won't compile
until they add the geo methods. `iceberg` is v0 so semver doesn't strictly
bind, but it's churn we're choosing.
Worth noting Java deliberately keeps geo out of `BoundExpressionVisitor` and
dispatches it through standalone `GeospatialPredicateEvaluators` instead.
Either we follow that and keep bbox eval outside the visitor via a
type-assertion path, or we accept the break here — but I'd make it a deliberate
call, and if we keep it in the visitor, either note it in the changelog or ship
a `DefaultBoundBooleanExprVisitor` with no-op geo methods embedders can mix in.
wdyt?
##########
expr_bbox_test.go:
##########
@@ -0,0 +1,179 @@
+// 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 iceberg_test
+
+import (
+ "encoding/json"
+ "math"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+func geoTestSchema() *iceberg.Schema {
+ return iceberg.NewSchema(1,
+ iceberg.NestedField{ID: 1, Name: "geom", Type:
iceberg.GeometryType{}, Required: false},
+ iceberg.NestedField{ID: 2, Name: "geog", Type:
iceberg.GeographyType{}, Required: false},
+ iceberg.NestedField{ID: 3, Name: "num", Type:
iceberg.PrimitiveTypes.Int32, Required: false},
+ )
+}
+
+func TestBBoxIntersectsConstruction(t *testing.T) {
+ bbox := iceberg.BoundingBox{MinX: 0, MinY: 0, MaxX: 10, MaxY: 10}
+ pred := iceberg.BBoxIntersects(iceberg.Reference("geom"), bbox)
+
+ assert.Equal(t, iceberg.OpBBoxIntersects, pred.Op())
+ assert.Equal(t, iceberg.Reference("geom"), pred.Term())
+ assert.Contains(t, pred.String(), "BBoxIntersects")
+
+ // Negation flips the operation and round-trips back.
+ neg := pred.Negate()
+ assert.Equal(t, iceberg.OpBBoxNotIntersects, neg.Op())
+ assert.True(t, pred.Equals(neg.Negate()))
+ assert.False(t, pred.Equals(neg))
+}
+
+func TestBBoxIntersectsNilTermPanics(t *testing.T) {
+ assert.Panics(t, func() {
+ iceberg.BBoxIntersects(nil, iceberg.BoundingBox{})
+ })
+}
+
+func TestBoundingBoxValid(t *testing.T) {
+ nan := math.NaN()
+ inf := math.Inf(1)
+ tests := []struct {
+ name string
+ bbox iceberg.BoundingBox
+ want bool
+ }{
+ {"zero", iceberg.BoundingBox{}, true},
+ {"normal", iceberg.BoundingBox{MinX: 0, MinY: 0, MaxX: 10,
MaxY: 10}, true},
+ {"open half-plane via inf", iceberg.BoundingBox{MinX: -inf,
MinY: -inf, MaxX: inf, MaxY: inf}, true},
+ {"inverted x", iceberg.BoundingBox{MinX: 10, MinY: 0, MaxX: 0,
MaxY: 10}, false},
+ {"inverted y", iceberg.BoundingBox{MinX: 0, MinY: 10, MaxX: 10,
MaxY: 0}, false},
+ {"nan min", iceberg.BoundingBox{MinX: nan, MinY: 0, MaxX: 10,
MaxY: 10}, false},
+ {"nan max", iceberg.BoundingBox{MinX: 0, MinY: 0, MaxX: 10,
MaxY: nan}, false},
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ assert.Equal(t, tt.want, tt.bbox.Valid())
+ })
+ }
+}
+
+func TestBBoxIntersectsInvalidBoxPanics(t *testing.T) {
+ // An inverted or NaN box would silently mis-prune, so construction
rejects it.
+ assert.Panics(t, func() {
+ iceberg.BBoxIntersects(iceberg.Reference("geom"),
+ iceberg.BoundingBox{MinX: 10, MinY: 0, MaxX: 0, MaxY:
10})
+ })
+ assert.Panics(t, func() {
+ iceberg.BBoxIntersects(iceberg.Reference("geom"),
+ iceberg.BoundingBox{MinX: math.NaN(), MinY: 0, MaxX:
10, MaxY: 10})
+ })
+}
+
+func TestBBoxIntersectsBind(t *testing.T) {
+ sc := geoTestSchema()
+ bbox := iceberg.BoundingBox{MinX: 1, MinY: 2, MaxX: 3, MaxY: 4}
+
+ for _, name := range []string{"geom", "geog"} {
Review Comment:
This loop doesn't wrap the body in `t.Run`, so a failure on `"geom"` aborts
before `"geog"` runs — and the rest of the PR uses subtests. I'd wrap it in
`t.Run(name, func(t *testing.T){...})` so both geo types are exercised
independently.
##########
table/evaluators_geo_test.go:
##########
@@ -0,0 +1,168 @@
+// 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 table
+
+import (
+ "encoding/binary"
+ "math"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// geoBound2D encodes a single bound point in the Iceberg geospatial
single-value
+// serialization: little-endian float64 X then Y (16 bytes).
+func geoBound2D(x, y float64) []byte {
+ b := make([]byte, 16)
+ binary.LittleEndian.PutUint64(b[0:], math.Float64bits(x))
+ binary.LittleEndian.PutUint64(b[8:], math.Float64bits(y))
+
+ return b
+}
+
+var geoMetricsSchema = iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "geom", Type: iceberg.GeometryType{},
Required: false},
+ iceberg.NestedField{ID: 2, Name: "geog", Type: iceberg.GeographyType{},
Required: false},
+)
+
+// TestInclusiveMetricsBBoxIntersects verifies that a data file is dropped only
+// when its geometry bounds cannot intersect the query bbox.
+func TestInclusiveMetricsBBoxIntersects(t *testing.T) {
Review Comment:
These are solid unit tests for the metrics evaluator, but everything here
calls `newInclusiveMetricsEvaluator` directly on a hand-built `mockDataFile`.
Nothing drives the predicate through a real
`table.Scan(WithFilter(BBoxIntersects(...)))` — binding, column-name
translation, projection, rewrite — into the manifest→metrics path.
That gap is exactly where the two blockers I flagged live (the substrait
panic and the `AlwaysFalse` drop both happen in translation, upstream of this
evaluator). A scan-level test with two data files, one overlapping and one
disjoint, asserting only the overlapping file lands in the scan tasks, would
have caught both and would keep them caught. Could we add one?
##########
exprs.go:
##########
@@ -1132,3 +1143,159 @@ func rejectTransformTerm(term BoundTerm) error {
return nil
}
+
+// BoundingBox is a planar (XY) query bounding box used by the geospatial
+// BBoxIntersects predicate. MinX/MinY are the lower-left corner and MaxX/MaxY
+// the upper-right corner (X is longitude/easting, Y is latitude/northing).
+//
+// The box is two-dimensional: pruning only compares the X and Y extents of a
+// geometry column's bounds, which is all the Iceberg spec requires for
+// bbox-based data skipping. Z/M extents in a column's bounds are ignored.
+type BoundingBox struct {
+ MinX, MinY, MaxX, MaxY float64
+}
+
+func (b BoundingBox) String() string {
+ return fmt.Sprintf("BoundingBox(minX=%g, minY=%g, maxX=%g, maxY=%g)",
+ b.MinX, b.MinY, b.MaxX, b.MaxY)
+}
+
+// Equals reports whether two bounding boxes have identical extents.
+func (b BoundingBox) Equals(other BoundingBox) bool {
+ return b.MinX == other.MinX && b.MinY == other.MinY &&
+ b.MaxX == other.MaxX && b.MaxY == other.MaxY
+}
+
+// Valid reports whether the box is well-formed: no coordinate is NaN and the
+// minimum of each axis does not exceed its maximum. An infinite bound is
allowed
+// (an open half-plane). An invalid box would silently mis-prune - a NaN makes
+// every intersection test false (pruning matching files), and an inverted box
+// (min > max) reports the wrong overlap - so BBoxIntersects rejects one.
+func (b BoundingBox) Valid() bool {
+ if math.IsNaN(b.MinX) || math.IsNaN(b.MinY) ||
+ math.IsNaN(b.MaxX) || math.IsNaN(b.MaxY) {
+ return false
+ }
+
+ return b.MinX <= b.MaxX && b.MinY <= b.MaxY
+}
+
+// isGeoType reports whether t is a geometry or geography type, the only types
a
+// bbox predicate may bind to.
+func isGeoType(t Type) bool {
+ switch t.(type) {
+ case GeometryType, GeographyType:
+ return true
+ default:
+ return false
+ }
+}
+
+// BBoxIntersects constructs an unbound geospatial predicate that matches rows
+// whose geometry/geography value's bounding box intersects the query box. It
is
+// used to prune data files whose stored geo bounds cannot overlap the query
+// region; the spec only requires bbox-based pruning, so full geometric
predicate
+// evaluation (ST_Intersects/ST_Within) remains a query-engine concern.
+//
+// Panics if the term is nil or the bbox is not Valid (NaN coordinate or an
+// inverted min/max), either of which would cause silent mis-pruning.
+func BBoxIntersects(t UnboundTerm, bbox BoundingBox) UnboundPredicate {
Review Comment:
This is the outermost user-facing entry point, and a caller deriving a box
from user input (a map viewport, a request param) can't guard construction
without wrapping in `recover()`. I'd lean toward `BBoxIntersects(t, bbox)
(UnboundPredicate, error)`.
`NewNot`/`NewAnd` panic too, so there's precedent for the style — if we keep
the panic, I'd at least document the panic contract in the godoc so it isn't a
surprise. wdyt?
##########
expr_json.go:
##########
@@ -195,6 +195,14 @@ func (bsp *boundSetPredicate[T]) MarshalJSON() ([]byte,
error) {
return marshalSetPredicate(bsp.op, bsp.term, bsp.lits.Members())
}
+// Geospatial bbox predicates have no representation in the REST expression
JSON
+// grammar; they are used only for local scan planning. Report an error rather
+// than silently emitting an empty object.
+func (u *unboundBBoxPredicate) MarshalJSON() ([]byte, error) { return nil,
errBBoxNotSerializable }
+func (b *boundBBoxPredicate) MarshalJSON() ([]byte, error) { return nil,
errBBoxNotSerializable }
+
+var errBBoxNotSerializable = fmt.Errorf("%w: geospatial bbox predicates cannot
be serialized to REST expression JSON", ErrInvalidArgument)
Review Comment:
Small thing — the sentinel is declared after the methods that use it, which
reads backward, and the repo keeps wrapped sentinel errors in `errors.go`. I'd
move it there (or at least above these methods) to match convention.
##########
visitors.go:
##########
@@ -385,6 +391,20 @@ func (e *exprEvaluator) VisitNotStartsWith(term BoundTerm,
lit Literal) bool {
return !e.VisitStartsWith(term, lit)
}
+// VisitBBoxIntersects evaluates a geospatial bbox predicate at the row level.
+// Row-level geometric evaluation (parsing each row's WKB and testing its
+// bounding box) is a query-engine concern - the Iceberg spec only requires
+// bbox-based data skipping, which happens in the metrics/manifest evaluators.
+// Here the predicate conservatively keeps every row so it never drops a row
that
+// an engine's own ST_Intersects would retain.
+func (e *exprEvaluator) VisitBBoxIntersects(BoundTerm, BoundingBox) bool {
+ return true
+}
+
+func (e *exprEvaluator) VisitBBoxNotIntersects(BoundTerm, BoundingBox) bool {
Review Comment:
`VisitBBoxIntersects` just above has a solid comment on why it keeps every
row; `VisitBBoxNotIntersects` returns `true` with none. The `true` is correct
(bounds can't prove a not-intersects), but it's exactly the kind of bare
`return true` a future maintainer might "tidy" to `false` and start dropping
rows. A one-line comment mirroring the one above would protect it.
##########
exprs.go:
##########
@@ -1132,3 +1143,159 @@ func rejectTransformTerm(term BoundTerm) error {
return nil
}
+
+// BoundingBox is a planar (XY) query bounding box used by the geospatial
+// BBoxIntersects predicate. MinX/MinY are the lower-left corner and MaxX/MaxY
+// the upper-right corner (X is longitude/easting, Y is latitude/northing).
+//
+// The box is two-dimensional: pruning only compares the X and Y extents of a
+// geometry column's bounds, which is all the Iceberg spec requires for
+// bbox-based data skipping. Z/M extents in a column's bounds are ignored.
+type BoundingBox struct {
Review Comment:
This box is XY-only, which is fine for the pruning the spec requires — no
correctness issue, inclusive eval just keeps more files than Java would for
Z/M-bounded data. My concern is forward-compat: Java's `BoundingBox` carries
optional Z and M, and once this struct ships in the public `iceberg` package,
its `Valid()`/`Equals()` semantics are effectively frozen.
Since callers build these as literals, adding optional `*float64`
`MinZ`/`MaxZ`/`MinM`/`MaxM` later is mostly additive, but the validity/equality
contract would shift under existing callers. I'd at least decide now whether to
reserve those fields so the door stays open. Not blocking — just cheaper to
settle before it's public. 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: [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]
