laskoviymishka commented on code in PR #1675:
URL: https://github.com/apache/iceberg-go/pull/1675#discussion_r3966545794
##########
table/internal/partition_predicate.go:
##########
@@ -151,6 +196,110 @@ 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 isIdentityTransform(transform) {
+ return ref
+ }
+ if isVoidTransform(transform) {
Review Comment:
This branch is dead with the current callers. `partitionTerm` is only
reached for fields where `fr.isVoid` is false, and that's set precisely when
`isVoidTransform` is true up in the field loop, so a void transform never flows
down here.
I'd drop the `isVoidTransform` arm and let it fall through to the identity
check plus the `NewUnboundTransform` default. wdyt?
##########
table/internal/partition_predicate.go:
##########
@@ -39,46 +39,69 @@ import (
// result is an OR across distinct partitions, each clause an AND across the
// spec's fields:
//
-// source == value when the partition value is present
-// IsNaN(source) when the value is a floating-point NaN (x == NaN is
never true)
-// IsNull(source) when the partition value is absent or nil
+// transform(source) == value when the partition value is present
+// IsNaN(transform(source)) when the value is a floating-point NaN (x ==
NaN is never true)
+// IsNull(transform(source)) when the partition value is absent or nil
//
// Duplicate tuples collapse to a single clause, and an empty input yields
// AlwaysFalse (matching nothing). Callers are expected to pass a partitioned
// spec; dynamic partition overwrite rejects unpartitioned tables upstream.
+// Void fields are also accepted when their source column has been dropped (the
+// spec represents those tombstones with source ID 0), because void always
+// produces a null partition value and does not need a source column.
//
-// Because only identity transforms are accepted (see below), the partition
-// value equals the source-column value, so "source == value" selects exactly
-// the rows in that partition. Non-identity transforms (bucket, truncate, the
-// time transforms) cannot be matched by a source-column predicate and need
-// partition-level matching instead; they are rejected here and tracked as a
-// follow-up under issue #1215.
+// The transform is kept in the row predicate so the match is evaluated against
+// the partition value rather than comparing the post-transform value directly
+// with the source column. This is phase 1 of issue #1216. The current
overwrite
+// path cannot execute non-identity predicates for partial-file rewrites
because
+// source-column metrics are conservative and the Substrait row-filter
converter
+// rejects transformed terms; partition-level strict matching is still needed
+// before this helper can drive those rewrites (tracked in issue #1215).
func BuildPartitionMatchPredicate(spec iceberg.PartitionSpec, schema
*iceberg.Schema, partitions []map[int]any) (iceberg.BooleanExpression, error) {
type fieldRef struct {
- id int
- name string
+ id int
+ name string
+ transform iceberg.Transform
+ resultType iceberg.Type
+ isVoid bool
}
var fields []fieldRef
for _, f := range spec.Fields() {
- if _, ok := f.Transform.(iceberg.IdentityTransform); !ok {
- return nil, fmt.Errorf("%w: dynamic partition overwrite
supports identity-transform partition fields only, got %s on %q (tracked in
https://github.com/apache/iceberg-go/issues/1215)",
- iceberg.ErrNotImplemented, f.Transform, f.Name)
- }
-
- // Identity transforms always have exactly one source column.
+ // Partition transforms currently have exactly one source
column.
if len(f.SourceIDs) != 1 {
- return nil, fmt.Errorf("%w: identity partition field %q
must have exactly one source id, got %d",
+ return nil, fmt.Errorf("%w: partition field %q must
have exactly one source id, got %d",
iceberg.ErrInvalidArgument, f.Name,
len(f.SourceIDs))
}
+ if isVoidTransform(f.Transform) {
+ // A void field can survive source-column removal as a
source-less
+ // tombstone. Its output is always null, so resolving
or binding the
+ // source would add no information and would reject
source ID 0.
+ if _, err := f.Transform.MarshalText(); err != nil {
Review Comment:
This MarshalText call can't fail. `VoidTransform.MarshalText` always returns
`("void", nil)`, so the error branch is unreachable.
The twin call in the non-void path just below at line 98 has the same
problem: anything that made it past `Bind` is already representable. I'd drop
both error checks.
##########
table/internal/partition_predicate.go:
##########
@@ -151,6 +196,110 @@ 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 isIdentityTransform(transform) {
+ return ref
+ }
+ if isVoidTransform(transform) {
+ // Use the value form so binding the null predicate can fold it
to
+ // AlwaysTrue. Pointer forms are accepted by the Transform
interface too.
+ return iceberg.NewUnboundTransform(iceberg.VoidTransform{}, ref)
+ }
+
+ return iceberg.NewUnboundTransform(transform, ref)
+}
+
+func isIdentityTransform(transform iceberg.Transform) bool {
+ switch t := transform.(type) {
+ case iceberg.IdentityTransform:
+ return true
+ case *iceberg.IdentityTransform:
+ return t != nil
+ default:
+ return false
+ }
+}
+
+func isVoidTransform(transform iceberg.Transform) bool {
+ switch t := transform.(type) {
+ case iceberg.VoidTransform:
+ return true
+ case *iceberg.VoidTransform:
+ return t != nil
+ default:
+ return false
+ }
+}
+
+func isTruncateTransform(transform iceberg.Transform) bool {
+ switch t := transform.(type) {
+ case iceberg.TruncateTransform:
+ return true
+ case *iceberg.TruncateTransform:
+ return t != nil
+ default:
+ return false
+ }
+}
+
+func validatePartitionValue(transform iceberg.Transform, resultType
iceberg.Type, lit iceberg.Literal) (iceberg.Literal, error) {
+ normalized, err := lit.To(resultType)
+ if err != nil {
+ return nil, fmt.Errorf("%w: partition value type %s cannot be
converted to transform result type %s: %v",
+ iceberg.ErrInvalidArgument, lit.Type(), resultType, err)
+ }
+
+ switch normalized.(type) {
+ case iceberg.AboveMaxLiteral, iceberg.BelowMinLiteral:
+ return nil, fmt.Errorf("%w: partition value %s is outside
transform result type %s",
+ iceberg.ErrInvalidArgument, normalized, resultType)
+ }
+
+ switch t := transform.(type) {
+ case iceberg.BucketTransform:
+ if err := validateBucketPartitionValue(t.NumBuckets,
normalized); err != nil {
+ return nil, err
+ }
+ case *iceberg.BucketTransform:
+ if t == nil {
Review Comment:
Same story as `partitionTerm` here: both of these are unreachable.
`validatePartitionValue` only runs after the `isVoid` guard, so the
`VoidTransform` case a few lines down never fires, and a typed-nil
`*BucketTransform` is already rejected by `Bind` before we get here (the
typed-nil-bucket test confirms it), so the `t == nil` guard can't trip either.
I'd drop both and keep just the value-form bucket case.
##########
table/internal/partition_predicate.go:
##########
@@ -151,6 +196,110 @@ 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 isIdentityTransform(transform) {
+ return ref
+ }
+ if isVoidTransform(transform) {
+ // Use the value form so binding the null predicate can fold it
to
+ // AlwaysTrue. Pointer forms are accepted by the Transform
interface too.
+ return iceberg.NewUnboundTransform(iceberg.VoidTransform{}, ref)
+ }
+
+ return iceberg.NewUnboundTransform(transform, ref)
+}
+
+func isIdentityTransform(transform iceberg.Transform) bool {
+ switch t := transform.(type) {
+ case iceberg.IdentityTransform:
+ return true
+ case *iceberg.IdentityTransform:
+ return t != nil
+ default:
+ return false
+ }
+}
+
+func isVoidTransform(transform iceberg.Transform) bool {
+ switch t := transform.(type) {
+ case iceberg.VoidTransform:
+ return true
+ case *iceberg.VoidTransform:
+ return t != nil
+ default:
+ return false
+ }
+}
+
+func isTruncateTransform(transform iceberg.Transform) bool {
+ switch t := transform.(type) {
+ case iceberg.TruncateTransform:
+ return true
+ case *iceberg.TruncateTransform:
+ return t != nil
+ default:
+ return false
+ }
+}
+
+func validatePartitionValue(transform iceberg.Transform, resultType
iceberg.Type, lit iceberg.Literal) (iceberg.Literal, error) {
+ normalized, err := lit.To(resultType)
+ if err != nil {
+ return nil, fmt.Errorf("%w: partition value type %s cannot be
converted to transform result type %s: %v",
Review Comment:
The inner conversion error goes in with `%v`, so it lands in the message
text but not the error chain, and `errors.Is/As` can't reach the cause.
I'd switch it to `%w`, or drop the fragment if only the sentinel is meant to
be unwrappable.
--
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]
