zeroshade commented on code in PR #1675:
URL: https://github.com/apache/iceberg-go/pull/1675#discussion_r3866125685
##########
table/internal/partition_predicate.go:
##########
@@ -77,8 +70,12 @@ func BuildPartitionMatchPredicate(spec
iceberg.PartitionSpec, schema *iceberg.Sc
return nil, fmt.Errorf("%w: partition field %q
references unknown source id %d",
iceberg.ErrInvalidArgument, f.Name,
f.SourceIDs[0])
}
+ if !f.Transform.CanTransform(src.Type) {
+ return nil, fmt.Errorf("%w: transform %s cannot be
applied to source field %q of type %s",
+ iceberg.ErrInvalidArgument, f.Transform,
src.Name, src.Type)
+ }
- fields = append(fields, fieldRef{id: f.FieldID, name: src.Name})
+ fields = append(fields, fieldRef{id: f.FieldID, name: src.Name,
transform: f.Transform})
Review Comment:
`FindFieldByID` returns the field's leaf name, so a partition source such as
`location.category` is stored as `category`. The resulting `truncate(category)`
predicate cannot bind to the schema; I reproduced the `invalid schema: could
not bind reference 'category'` failure 10/10 times. If a top-level field shares
that leaf name, this can instead bind the wrong source and select the wrong
rows for overwrite.
Please obtain the full path with `schema.FindColumnName(f.SourceIDs[0])`
while retaining `src` for type validation, and add nested transformed-source
coverage.
##########
table/internal/partition_predicate.go:
##########
@@ -151,6 +148,15 @@ func BuildPartitionMatchPredicate(spec
iceberg.PartitionSpec, schema *iceberg.Sc
return result, nil
}
+func partitionTerm(transform iceberg.Transform, name string)
iceberg.UnboundTerm {
+ ref := iceberg.Reference(name)
+ if _, ok := transform.(iceberg.IdentityTransform); ok {
Review Comment:
Pointer forms of built-in transforms satisfy `Transform` and are accepted
elsewhere, but this assertion recognizes only the value form. With
`&IdentityTransform{}`, the builder emits `identity(id) == 5` instead of the
promised unchanged `id == 5`; reproduced 10/10 times. That unnecessarily enters
the transformed-filter path, which `prepareBatchFilter` rejects.
Please recognize both pointer and value identity transforms and add a
regression. This is adjacent to, but distinct from, the pointer-transform
equality issue on dependency PR #1679.
--
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]
