laskoviymishka commented on code in PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#discussion_r3577774200
##########
transforms.go:
##########
@@ -77,7 +84,9 @@ func ParseTransform(s string) (Transform, error) {
return HourTransform{}, nil
}
- return nil, fmt.Errorf("%w: %s", ErrInvalidTransform, s)
+ // Unknown transform: v3 readers must load these and ignore them when
+ // filtering instead of failing. Keep the original string so it
round-trips.
+ return UnknownTransform{name: s}, nil
Review Comment:
Since `ParseTransform` now keeps the original casing here and `Equals`
compares `name` byte-for-byte, `"Custom_V2[3]"` and `"custom_v2[3]"` become two
distinct `UnknownTransform`s — the new test even pins that behavior. On a
partition spec that'd let two effectively-identical unknown fields coexist.
Probably fine in practice, but I'd either normalize the stored name or leave a
comment noting the case-sensitivity is intentional for round-trip fidelity.
##########
table/sorting_test.go:
##########
@@ -262,6 +280,33 @@ func TestUnmarshalInvalidSortTransform(t *testing.T) {
]
}`
+ var order table.SortOrder
+ err := json.Unmarshal([]byte(j), &order)
+ require.NoError(t, err)
+ require.Equal(t, 3, order.Len())
+
+ var first table.SortField
+ for i, f := range order.Fields() {
Review Comment:
Tiny thing — this loop only exists to grab field 0. `order.Fields()[0]` (or
a `Field(0)` accessor if we add one) reads cleaner.
##########
partitions_test.go:
##########
@@ -191,6 +191,47 @@ func
TestPartitionSpec_MarshalTextRejectsInvalidBucketTransform(t *testing.T) {
require.ErrorContains(t, err, "numBuckets > 0")
}
+// A v3 table may use a partition transform this implementation doesn't know.
+// Readers must load it and preserve it on write, ignoring it when filtering.
+func TestPartitionFieldUnknownTransformRoundTrip(t *testing.T) {
Review Comment:
Nice that this proves the field round-trips. Could we add a sibling test
that unmarshals a full table-metadata JSON — partition specs and all — with an
unknown transform and asserts it loads clean? That's the case that actually
matters for the read goal, and the one thing that proves loading a v3 table
doesn't trip `validateTransform` anywhere on the metadata path. The field-level
test doesn't exercise that.
##########
table/sorting.go:
##########
@@ -320,6 +320,10 @@ func newSortOrder(orderID int, fields []SortField,
validateSourceIDs bool) (Sort
return SortOrder{}, fmt.Errorf("%w: sort field
at index %d has invalid source IDs: %v",
ErrInvalidSortSourceID, idx, err)
}
+ if u, ok := field.Transform.(iceberg.UnknownTransform);
ok {
Review Comment:
Two things on this guard.
First, it's stricter than the spec and than Java. The spec only forbids
writing partition specs with unknown transforms, and Java's `SortOrder` builder
doesn't reject unknown sort transforms (`canTransform` stays true). A v3 sort
order round-tripped through `NewSortOrder` — say during `UpdateSortOrder` —
would get wrongly rejected here. I think this ties into tanmayrauth's
sort-order thread, so I want to make sure we actually want to reject on write
at all, rather than tolerate it like Java does.
Second, if we do keep it: it's inside the `validateSourceIDs` block, so the
JSON unmarshal path (`validateSourceIDs=false`) skips it entirely, and
`MetadataBuilder.AddSortOrder` bypasses it too. That's an inconsistent guard.
I'd pull it out of the `validateSourceIDs` branch so write and load agree. wdyt?
##########
partitions.go:
##########
@@ -291,6 +291,8 @@ func validateTransform(transform Transform) error {
return t.validateNumBuckets()
case *BucketTransform:
return t.validateNumBuckets()
+ case UnknownTransform:
Review Comment:
This case is right for a direct write, but it also fires on the preserve
path. `UpdateSpec.Apply` rebuilds the whole spec through
`NewPartitionSpecOpts`, so every existing field — including a passthrough
unknown-transform one we loaded from a v3 table — gets re-validated here. That
means once a table has an unknown partition field, we can't evolve its spec at
all: renaming or dropping an unrelated field fails with `ErrInvalidTransform`
at commit.
I'd only validate the fields being newly added, and route the preserved
fields through a non-validating construction. Java rejects at the `UpdateSpec`
constructor with a clear message rather than an opaque commit-time error, so an
early guard there would be friendlier too. A test that evolves a spec on an
unknown-transform table would lock this down. wdyt?
--
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]
