tanmayrauth commented on PR #1597:
URL: https://github.com/apache/iceberg-go/pull/1597#issuecomment-5121891351
@laskoviymishka Thanks for the thorough review — really useful pass. All
four blockers plus every inline item are addressed.
Blockers
- Column-name translation (the crux): columnNameTranslator.VisitBound now
special-cases *boundBBoxPredicate → AlwaysTrue at the top, before both the
FindColumnName not-found return and the switch. That closes both directions in
one guard — the substrait panic when the geo column is present, and the
AlwaysFalse-drops-every-row case when the file predates the column.
- Inverted bounds: GeoBoundsXY now rejects lo > hi on X/Y right after the
NaN check, so a malformed third-party bound makes the file unprunable rather
than silently pruned.
- End-to-end test: added TestScanPrunesDisjointGeometryFile (real
Scan(WithRowFilter(BBoxIntersects(...))) over an overlapping + a disjoint file,
asserting only the overlapping one survives PlanFiles). Since the two blockers
actually live in TranslateColumnNames/substrait on the record-read path (which
PlanFiles doesn't reach), I also added TestBBoxTranslateColumnNames (present +
absent column) and a substrait backstop test to pin those two directly —
verified they fail if the VisitBound guard is reverted.
- Visitor interface: made the deliberate call to follow Java — the two geo
methods are removed from BoundBooleanExprVisitor and moved to a new optional
BoundGeospatialExprVisitor[T], dispatched via
type-assertion in VisitBoundPredicate. No external break; internal
visitors satisfy it for free, with var _ BoundGeospatialExprVisitor[…] =
(*…)(nil) assertions keeping that compile-checked.
Smaller items: substrait backstop now panics with a wrapped
ErrNotImplemented (typed/wrappable, though unreachable now that bbox is dropped
upstream); errBBoxNotSerializable moved to errors.go as exported
ErrBBoxNotSerializable; the geom/geog test loop uses t.Run;
VisitBBoxNotIntersects got a comment mirroring the intersects one; BoundingBox
stays XY-only with the forward-compat contract documented (Z/M additive
later); and BBoxIntersects keeps the panic for consistency with the other
predicate constructors, with the contract documented — happy to switch to an
error return if you'd prefer.
--
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]
