zeroshade commented on code in PR #1675:
URL: https://github.com/apache/iceberg-go/pull/1675#discussion_r4065554947
##########
table/internal/partition_predicate.go:
##########
@@ -39,46 +39,62 @@ 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 explicitly nil
+// Missing partition field IDs are rejected with ErrInvalidArgument.
//
// 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 retained in the row predicate so the post-transform
+// partition value is compared against transform(source), not against the raw
+// 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.
+ fields = append(fields, fieldRef{id: f.FieldID, name:
f.Name, transform: f.Transform, isVoid: true})
+
+ continue
+ }
- src, ok := schema.FindFieldByID(f.SourceIDs[0])
+ sourceName, ok := schema.FindColumnName(f.SourceIDs[0])
if !ok {
return nil, fmt.Errorf("%w: partition field %q
references unknown source id %d",
iceberg.ErrInvalidArgument, f.Name,
f.SourceIDs[0])
}
-
- fields = append(fields, fieldRef{id: f.FieldID, name: src.Name})
+ bound, err := iceberg.NewUnboundTransform(f.Transform,
iceberg.Reference(sourceName)).Bind(schema, true)
Review Comment:
`Bind` validates nil/unknown transforms and source compatibility, but not
transform *parameters*: `BucketTransform.NumBuckets` and
`TruncateTransform.Width` are never range-checked, and `NewPartitionSpec`
deliberately accepts unvalidated fields.
So `bucket[0]` or `truncate[0]` paired with an explicit nil partition value
builds successfully as `IsNull(transform(value))`. The invalid transform's
`Apply` returns an invalid optional for every non-null source, which reads as
null, so the predicate evaluates true against non-null input — probing
`int32(1)` matched for both. Once the overwrite path is wired, that is an
all-row match where the caller intended a single partition, i.e. deleting rows
outside the target.
Restoring the parameter check the 09-03 cleanup removed (the former
`MarshalText` validation, or an equivalent validator) would close it.
Non-blocking today only because nothing reaches it yet.
##########
table/internal/partition_predicate.go:
##########
@@ -151,6 +189,92 @@ 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
+ }
+
+ 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: %w",
+ 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 {
Review Comment:
This case covers only the value form:
```go
switch t := transform.(type) {
case iceberg.BucketTransform:
```
The non-nil pointer case went away with the 09-03 cleanup, so
`&BucketTransform{NumBuckets: 4}` with the impossible partition value `4`
passes validation and yields a predicate that can never match. Under-pruning is
perf-only rather than a correctness hazard, which is the other reason I'm not
blocking — but the asymmetry between `BucketTransform` and `*BucketTransform`
will surprise someone eventually.
If you pick this up, three regressions would cover it: nil-valued
`bucket[0]`, nil-valued `truncate[0]`, and `&BucketTransform{NumBuckets: 4}`
with partition value `4`.
--
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]
