zeroshade commented on code in PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#discussion_r3732110097
##########
expr_json.go:
##########
@@ -579,6 +579,11 @@ func decodeTerm(raw json.RawMessage) (UnboundTerm, error) {
if err != nil {
return nil, fmt.Errorf("%w: cannot parse transform
term: %s", ErrInvalidArgument, err)
}
+ // Unknown transforms are tolerated in partition/sort metadata,
but a
+ // filter expression referencing one can't be evaluated.
+ if _, ok := tf.(UnknownTransform); ok {
Review Comment:
This rejection has no test coverage at all. It is a small addition, but it
is the only thing standing between an unknown transform and a malformed REST
filter payload.
Suggested fix: add a `decodeTerm` case feeding a term whose transform is
unknown and assert `ErrInvalidArgument`.
##########
transforms.go:
##########
@@ -230,6 +246,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 }
Review Comment:
Non-blocking: because `CanTransform` is unconditionally true,
`SortOrder.CheckCompatibility` silently accepts any source type for an unknown
transform. That is the right runtime behavior — we genuinely cannot know — but
the doc comment reads as if compatibility were verified.
Suggested fix: note on this method that compatibility is *unverifiable* for
unknown transforms, not verified.
##########
transforms.go:
##########
@@ -230,6 +246,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 (t UnknownTransform) ToHumanStr(any) string { return t.name }
+
+func (t UnknownTransform) ToHumanStrType(Type, any) string { return t.name }
Review Comment:
**Blocking (part of the position-delete issue in the summary).**
`ToHumanStrType` and `ToHumanStr` (`:283`) return the transform *name*, so they
are constant for a given spec field regardless of the value passed in.
`PartitionSpec.PartitionToPath` (`partitions.go:710`) feeds this straight
into the writer key at `table/rolling_data_writer.go:348`. Two genuinely
different partition values therefore produce the identical path string, the map
lookup returns the writer created for the first one, and the new
`partitionValues` are discarded. The same collapse shows up in
`table/snapshots.go:459,474`, where `partition-summaries` and
`changed-partition-count` come out wrong.
Suggested fix: render the value rather than the transform name. Java's
`Transform#toHumanString` default renders the value and `UnknownTransform` does
not override it, so matching that behavior also fixes the summary counts and
makes it impossible for any future `PartitionToPath` caller to collide.
##########
transforms.go:
##########
@@ -230,6 +246,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 {
Review Comment:
Non-blocking: `iceberg.UnknownTransform{}` is a legal composite literal from
outside the package — all fields being unexported makes them unsettable, not
the struct unconstructible. That yields `name == ""`, which marshals to
`"transform": ""` and produces metadata that cannot be read back. `MarshalText`
returning no error also bypasses the `newSortOrder` backstop at
`table/sorting.go:321`.
Suggested fix: reject empty names in `validateTransform`
(`partitions.go:314`), or return an error from `MarshalText` when `name` is
empty.
##########
table/sorting.go:
##########
@@ -249,6 +253,11 @@ func (s SortOrder) Len() int {
return len(s.fields)
}
+// Field returns the sort field at index i.
+func (s SortOrder) Field(i int) SortField {
Review Comment:
Non-blocking: the new `Field(i)` returns `s.fields[i]` raw while `Fields()`
(`:242`) clones. `SortField.SourceIDs` is a slice, so a caller can mutate the
sort order's internals through the returned value — which breaks exactly the
invariant `TestSortOrderReturnsDefensiveCopies` asserts for `Fields()`.
Suggested fix: return `cloneSortField(s.fields[i])`, and document that an
out-of-range index panics.
##########
table/update_spec.go:
##########
@@ -86,6 +86,12 @@ func NewUpdateSpec(t *Transaction, caseSensitive bool)
*UpdateSpec {
nameToField := make(map[string]iceberg.PartitionField)
partitionSpec := t.tbl.Metadata().PartitionSpec()
for _, partitionField := range partitionSpec.Fields() {
+ if _, ok :=
partitionField.Transform.(iceberg.UnknownTransform); ok {
Review Comment:
Worth surfacing to users: this blanket rejection blocks *all* spec evolution
on a table with an unknown transform, including unrelated renames and drops.
You confirmed this matches Java and it is the safer default, so no change to
the behavior — but the rationale currently lives only in
`update_spec_test.go:64`.
Suggested fix: move that reasoning into the `NewUpdateSpec` doc comment,
where someone hitting the error will actually find it.
--
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]
