tanmayrauth commented on code in PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#discussion_r3574533090
##########
partitions.go:
##########
@@ -291,6 +291,8 @@ func validateTransform(transform Transform) error {
return t.validateNumBuckets()
case *BucketTransform:
return t.validateNumBuckets()
+ case UnknownTransform:
Review Comment:
Partition specs reject unknown transforms on write here, but the sort-order
path doesn't: newSortOrder (table/sorting.go:309) and CheckCompatibility
(table/sorting.go:334) accept them, and CheckCompatibility passes only because
UnknownTransform.CanTransform returns true unconditionally. An UnknownTransform
is un-evaluatable (Apply returns empty, Project returns nil), so a SortOrder
built with one commits an un-enforceable sort into metadata with no guard and
no test — downstream engines then read a declared sort order they can't
reproduce. Either mirror the partition-spec rejection on the sort-write path,
or add a test pinning the intended behavior.
##########
transforms.go:
##########
@@ -230,6 +239,49 @@ func (VoidTransform) Project(string, BoundPredicate)
(UnboundPredicate, error) {
return nil, nil
}
+// UnknownTransform is a placeholder for a partition or sort transform that
+// this implementation doesn't recognize. The v3 spec requires readers to load
+// tables that use unknown transforms and to ignore those fields when
+// filtering; writers must not commit a partition spec that uses one.
+type UnknownTransform struct {
+ name string
+}
+
+func (t UnknownTransform) MarshalText() ([]byte, error) {
+ return []byte(t.name), nil
+}
+
+func (t UnknownTransform) String() string { return t.name }
+
+// CanTransform assumes an unknown transform could apply to any type -- the
+// real applicability isn't known.
+func (UnknownTransform) CanTransform(Type) bool { return true }
+
+// ResultType is unknown, so report string, matching the Java reference.
+func (UnknownTransform) ResultType(Type) Type { return StringType{} }
+
+func (UnknownTransform) PreservesOrder() bool { return false }
+
+func (t UnknownTransform) Equals(other Transform) bool {
+ o, ok := other.(UnknownTransform)
+
+ return ok && t.name == o.name
+}
+
+// Apply can't be evaluated for an unknown transform.
+func (UnknownTransform) Apply(Optional[Literal]) Optional[Literal] {
+ return Optional[Literal]{}
+}
+
+func (UnknownTransform) ToHumanStr(any) string { return "null" }
+
+func (UnknownTransform) ToHumanStrType(Type, any) string { return "null" }
+
+// Project returns nil so scans don't prune on an unknown partition field.
+func (UnknownTransform) Project(string, BoundPredicate) (UnboundPredicate,
error) {
Review Comment:
This returning nil is the entire mechanism for "ignore the unknown field
when filtering" — inclusiveProjection in table/evaluators.go treats a nil
projection as AlwaysTrue — but there's no test for it. If a later change makes
Project return a non-nil predicate, or the projection loop stops treating nil
as AlwaysTrue, unknown partition fields start pruning again and the suite stays
green, silently dropping matching rows. Please add a test that builds a spec
with an unknown transform and asserts a predicate on that source column plans
without pruning.
##########
transforms.go:
##########
@@ -230,6 +239,49 @@ func (VoidTransform) Project(string, BoundPredicate)
(UnboundPredicate, error) {
return nil, nil
}
+// UnknownTransform is a placeholder for a partition or sort transform that
+// this implementation doesn't recognize. The v3 spec requires readers to load
+// tables that use unknown transforms and to ignore those fields when
+// filtering; writers must not commit a partition spec that uses one.
+type UnknownTransform struct {
+ name string
+}
+
+func (t UnknownTransform) MarshalText() ([]byte, error) {
+ return []byte(t.name), nil
+}
+
+func (t UnknownTransform) String() string { return t.name }
+
+// CanTransform assumes an unknown transform could apply to any type -- the
+// real applicability isn't known.
+func (UnknownTransform) CanTransform(Type) bool { return true }
+
+// ResultType is unknown, so report string, matching the Java reference.
+func (UnknownTransform) ResultType(Type) Type { return StringType{} }
+
+func (UnknownTransform) PreservesOrder() bool { return false }
+
+func (t UnknownTransform) Equals(other Transform) bool {
+ o, ok := other.(UnknownTransform)
+
+ return ok && t.name == o.name
+}
+
+// Apply can't be evaluated for an unknown transform.
+func (UnknownTransform) Apply(Optional[Literal]) Optional[Literal] {
+ return Optional[Literal]{}
+}
+
+func (UnknownTransform) ToHumanStr(any) string { return "null" }
Review Comment:
Returning the literal "null" here (and in ToHumanStrType on 278) is
indistinguishable from the human rendering of an actual null value, so
partition-summary/describe output for an unknown-transform field reads as
"null" and an operator can't tell "unknown transform" from "null partition
value." A distinct marker (e.g. the transform name) would avoid the ambiguity.
--
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]
