zeroshade commented on code in PR #1679:
URL: https://github.com/apache/iceberg-go/pull/1679#discussion_r3832049802
##########
exprs.go:
##########
@@ -790,7 +788,7 @@ type BoundLiteralPredicate interface {
func newBoundLiteralPredicate[T LiteralType](op Operation, term BoundTerm, lit
Literal) BoundPredicate {
return &boundLiteralPredicate[T]{
- op: op, term: term.(bound[T]),
+ op: op, term: term,
Review Comment:
**Blocking:** allowing a `BoundTransform` here exposes it to metrics and
bloom evaluators that still compare the transform-result literal with raw
source-column values from `term.Ref()`. I reproduced `truncate[3](category) ==
"boo"` incorrectly pruning a file containing `"books"`. More seriously, strict
metrics reports that every row matches `truncate[3](category) != "boo"` for a
required file containing only `"books"`, even though no row matches; filtered
deletion can therefore drop the entire file. Bloom pruning has the same
source-domain/transformed-domain mismatch. Please make all source-column
pruning consumers conservatively handle transformed terms or correctly
transform their statistics.
##########
exprs.go:
##########
@@ -838,7 +836,7 @@ func createBoundLiteralPredicate(op Operation, term
BoundTerm, lit Literal) (Bou
type boundLiteralPredicate[T LiteralType] struct {
op Operation
- term bound[T]
+ term BoundTerm
Review Comment:
**Blocking:** the Substrait converter is not transform-aware. Its reference,
literal, and set helpers all emit `term.Ref()` directly, so
`truncate[3](category) == "boo"` becomes `category == "boo"`, silently changing
the predicate. Bucket and temporal transforms can instead fail because the
source and result types differ. This path is used by scan filtering and
partial-file rewrites; please encode the transform in Substrait or reject
transformed predicates there rather than dropping it.
##########
visitors.go:
##########
@@ -406,8 +406,23 @@ func nullsFirstCmp[T LiteralType](cmp Comparator[T], v1,
v2 Optional[T]) int {
return cmp(v1.Val, v2.Val)
}
+func typedTermEval[T LiteralType](st StructLike, term BoundTerm) Optional[T] {
+ v := term.evalToLiteral(st)
+ if !v.Valid {
+ return Optional[T]{}
+ }
+
+ lit, ok := v.Val.(TypedLiteral[T])
Review Comment:
**Major:** this assertion makes `day` predicates unevaluable.
`DayTransform.ResultType` is `Date`, so binding selects `TypedLiteral[Date]`,
but `DayTransform.Apply` returns an `Int32Literal`. A direct `day(ts) ==
Date(0)` evaluation returns `type error: ... evaluated to literal type int,
expected date`. Please align the transform's evaluated literal with its
declared result type (and add coverage for `day` alongside bucket/truncate).
--
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]
