laskoviymishka commented on code in PR #1597: URL: https://github.com/apache/iceberg-go/pull/1597#discussion_r3677514814
########## table/evaluators_geo_test.go: ########## @@ -0,0 +1,256 @@ +// 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 ( + "bytes" + "context" + "encoding/binary" + "math" + "testing" + + "github.com/apache/iceberg-go" + iceio "github.com/apache/iceberg-go/io" + "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) { + // A file whose geometry column spans the box [0,0]-[10,10]. + lower, upper := geoBound2D(0, 0), geoBound2D(10, 10) + file := &mockDataFile{ + count: 2, + valueCounts: map[int]int64{1: 2}, + nullCounts: map[int]int64{1: 0}, + lowerBounds: map[int][]byte{1: lower}, + upperBounds: map[int][]byte{1: upper}, + } + + tests := []struct { + name string + bbox iceberg.BoundingBox + want bool // true => might match (kept), false => pruned + }{ + {"overlapping", iceberg.BoundingBox{MinX: 5, MinY: 5, MaxX: 15, MaxY: 15}, true}, + {"contained", iceberg.BoundingBox{MinX: 2, MinY: 2, MaxX: 3, MaxY: 3}, true}, + {"touching corner", iceberg.BoundingBox{MinX: 10, MinY: 10, MaxX: 20, MaxY: 20}, true}, + {"disjoint right", iceberg.BoundingBox{MinX: 11, MinY: 0, MaxX: 20, MaxY: 10}, false}, + {"disjoint diagonal", iceberg.BoundingBox{MinX: 20, MinY: 20, MaxX: 30, MaxY: 30}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + eval, err := newInclusiveMetricsEvaluator( + geoMetricsSchema, iceberg.BBoxIntersects(iceberg.Reference("geom"), tt.bbox), true, true) + require.NoError(t, err) + + got, err := eval(file) + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +// TestInclusiveMetricsBBoxIntersectsAllNull prunes a file whose geometry column +// is entirely null: no geometry can intersect any query box. +func TestInclusiveMetricsBBoxIntersectsAllNull(t *testing.T) { + file := &mockDataFile{ + count: 3, + valueCounts: map[int]int64{1: 3}, + nullCounts: map[int]int64{1: 3}, + } + + eval, err := newInclusiveMetricsEvaluator( + geoMetricsSchema, + iceberg.BBoxIntersects(iceberg.Reference("geom"), iceberg.BoundingBox{MinX: 0, MinY: 0, MaxX: 10, MaxY: 10}), + true, true) + require.NoError(t, err) + + got, err := eval(file) + require.NoError(t, err) + assert.False(t, got, "all-null geometry column must be pruned") +} + +// TestInclusiveMetricsBBoxIntersectsNoBounds keeps a file when the geometry +// column has no usable bounds. Geography columns never emit bounds, so a +// geography predicate can never prune - which is always safe. +func TestInclusiveMetricsBBoxIntersectsNoBounds(t *testing.T) { + file := &mockDataFile{ + count: 2, + valueCounts: map[int]int64{2: 2}, + nullCounts: map[int]int64{2: 0}, + } + + eval, err := newInclusiveMetricsEvaluator( + geoMetricsSchema, + iceberg.BBoxIntersects(iceberg.Reference("geog"), iceberg.BoundingBox{MinX: 100, MinY: 100, MaxX: 200, MaxY: 200}), + true, true) + require.NoError(t, err) + + got, err := eval(file) + require.NoError(t, err) + assert.True(t, got, "geography column has no bounds, so the file cannot be pruned") +} + +// TestInclusiveMetricsBBoxIntersectsGeographyWithBounds guards the antimeridian +// hazard: a geography file written by another engine can carry bounds, and those +// bounds may cross the antimeridian (lower_x > upper_x). A planar min/max compare +// would mis-handle the wrapped box and wrongly prune, so geography columns must +// never be pruned from bounds - even when the bounds look disjoint from the query. +func TestInclusiveMetricsBBoxIntersectsGeographyWithBounds(t *testing.T) { + file := &mockDataFile{ + count: 2, + valueCounts: map[int]int64{2: 2}, + nullCounts: map[int]int64{2: 0}, + // Bounds that look disjoint from the query box under a planar compare. + lowerBounds: map[int][]byte{2: geoBound2D(0, 0)}, + upperBounds: map[int][]byte{2: geoBound2D(10, 10)}, + } + + eval, err := newInclusiveMetricsEvaluator( + geoMetricsSchema, + iceberg.BBoxIntersects(iceberg.Reference("geog"), iceberg.BoundingBox{MinX: 100, MinY: 100, MaxX: 200, MaxY: 200}), + true, true) + require.NoError(t, err) + + got, err := eval(file) + require.NoError(t, err) + assert.True(t, got, "geography must not be pruned from planar bounds (antimeridian hazard)") +} + +// TestInclusiveMetricsBBoxNotIntersects never prunes: intersecting bounds don't +// prove any geometry lies outside the query box. +func TestInclusiveMetricsBBoxNotIntersects(t *testing.T) { + file := &mockDataFile{ + count: 2, + valueCounts: map[int]int64{1: 2}, + nullCounts: map[int]int64{1: 0}, + lowerBounds: map[int][]byte{1: geoBound2D(0, 0)}, + upperBounds: map[int][]byte{1: geoBound2D(10, 10)}, + } + + pred := iceberg.BBoxIntersects(iceberg.Reference("geom"), + iceberg.BoundingBox{MinX: 20, MinY: 20, MaxX: 30, MaxY: 30}).Negate() + eval, err := newInclusiveMetricsEvaluator(geoMetricsSchema, pred, true, true) + require.NoError(t, err) + + got, err := eval(file) + require.NoError(t, err) + assert.True(t, got, "not-intersects cannot prune from bounds alone") +} + +// TestScanPrunesDisjointGeometryFile drives a bbox filter through a real +// table.Scan - binding, column-name translation, projection, and the +// manifest->metrics path - rather than calling the metrics evaluator directly. +// It proves the end-to-end contract: a data file whose geometry bounds are +// disjoint from the query box is pruned, while an overlapping one survives. This +// is the path where a bbox predicate would otherwise either panic in substrait +// or be dropped to AlwaysFalse during column translation, so it guards both Review Comment: Small thing on the comment: `PlanFiles` doesn't run `ReadTasks`/`arrowScan`, so this doesn't actually exercise column-name translation or substrait — the "would otherwise panic in substrait or be dropped to AlwaysFalse during column translation" regressions are pinned by `TestBBoxPredicateConvertsToTypedError` and `TestBBoxTranslateColumnNames`, not here. What this test genuinely proves is the metrics-pruning path end-to-end, which is worth having on its own. I'd just retarget the comment to say that and point at the two unit tests for the translation/substrait regressions. ########## visitors.go: ########## @@ -140,6 +155,23 @@ func VisitBoundPredicate[T any](e BoundPredicate, visitor BoundBooleanExprVisito return visitor.VisitStartsWith(e.Term(), e.(BoundLiteralPredicate).Literal()) case OpNotStartsWith: return visitor.VisitNotStartsWith(e.Term(), e.(BoundLiteralPredicate).Literal()) + case OpBBoxIntersects, OpBBoxNotIntersects: + // Geospatial predicates are dispatched through the optional + // BoundGeospatialExprVisitor extension so visitors that predate (or don't + // care about) geo support are not forced to implement them. A visitor that + // doesn't implement it doesn't support geo predicates; panic here is + // recovered into an error by VisitExpr. + gv, ok := visitor.(BoundGeospatialExprVisitor[T]) Review Comment: The dispatch reads well, but this panic-when-the-visitor-doesn't-implement-the-extension is a new part of `VisitBoundPredicate`'s public contract, and the godoc says nothing about it. An external visitor that implements `BoundBooleanExprVisitor` without the geo methods and calls `VisitBoundPredicate` directly (not through `VisitExpr`) gets an unrecovered panic, and even the `VisitExpr` path returns an error the doc doesn't mention. I'd add a line to the godoc: if the predicate is a bbox op and the visitor doesn't implement `BoundGeospatialExprVisitor[T]`, this panics with a wrapped `ErrNotImplemented`, which `VisitExpr` recovers into a returned error. Cheap to write, saves a downstream adapter a surprise on upgrade. ########## visitors.go: ########## @@ -497,6 +552,18 @@ func (columnNameTranslator) VisitUnbound(pred UnboundPredicate) BooleanExpressio } func (c columnNameTranslator) VisitBound(pred BoundPredicate) BooleanExpression { + // A bbox predicate has no substrait/record-filter form; it is evaluated only + // during metrics-based file pruning, so the record filter must conservatively + // keep every row. It must be matched before the column-not-found early return, + // which would otherwise return AlwaysFalse and silently drop every row of a + // file that predates the geo column, and before the switch below, whose + // default panics on an unhandled predicate. Collapse it to always-true + // (mirrors exprEvaluator and sanitizeVisitor); this also keeps it out of + // substrait, where it would otherwise error. + if _, ok := pred.(*boundBBoxPredicate); ok { Review Comment: This guards the concrete `*boundBBoxPredicate`, but `BoundBBoxPredicate` is the exported interface — a downstream implementation or a test double satisfying it wouldn't match and would fall through to `FindColumnName` and the `default:` panic. Nothing external can construct one today (Bind always hands back the concrete type), so this is defensive rather than a live bug, but guarding on the interface (`if _, ok := pred.(BoundBBoxPredicate); ok`) is the same cost and future-proofs it. Same shape in `sanitizeVisitor.VisitBound` — and there the guard sits after `ref := Reference(pred.Term()...)`, so hoisting it above `ref` would match this ordering. wdyt? ########## errors.go: ########## @@ -35,4 +35,8 @@ var ( ErrBadLiteral = errors.New("invalid literal value") ErrInvalidBinSerialization = errors.New("invalid binary serialization") ErrResolve = errors.New("cannot resolve type") + // ErrBBoxNotSerializable is returned when marshaling a geospatial bbox + // predicate to REST expression JSON: bbox predicates exist only for local + // scan planning and have no representation in the REST expression grammar. + ErrBBoxNotSerializable = fmt.Errorf("%w: geospatial bbox predicates cannot be serialized to REST expression JSON", ErrInvalidArgument) Review Comment: `ErrBBoxNotSerializable` wraps `ErrInvalidArgument`, which means a caller doing `errors.Is(err, ErrInvalidArgument)` on a marshal result now matches a "this type has no JSON form" case as if it were a bad user argument. It's really a not-implemented/contract condition, not an argument error. I'd wrap `ErrNotImplemented` instead (or give it its own root sentinel) — consistent with how the substrait backstop already surfaces the same predicate. wdyt? ########## visitors_test.go: ########## @@ -172,6 +172,18 @@ func (e *FooBoundExprVisitor) VisitNotStartsWith(iceberg.BoundTerm, iceberg.Lite return e.visitHistory } +func (e *FooBoundExprVisitor) VisitBBoxIntersects(iceberg.BoundTerm, iceberg.BoundingBox) []string { Review Comment: `FooBoundExprVisitor` grew these two methods but nothing in the suite drives a bbox predicate through it, so the new type-assert-and-dispatch path in `VisitBoundPredicate` is uncovered — a refactor that broke the `BoundGeospatialExprVisitor[T]` assertion wouldn't be caught here. I'd add a case binding a `BBoxIntersects` predicate and asserting the visit history contains `BBOX_INTERSECTS`, plus one negative case: a bare `BoundBooleanExprVisitor` without the geo methods, passed a bbox expr through `VisitExpr`, asserting `errors.Is(err, ErrNotImplemented)`. That second one pins the load-bearing error path external callers actually hit. -- 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]
