laskoviymishka commented on code in PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#discussion_r3590554889
##########
transforms.go:
##########
@@ -230,6 +240,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] {
Review Comment:
this is the round-1 write-path concern, still open — flagging it here
because `Apply` returning an empty Optional (and `ToHumanStrType` just below
returning the transform name) is exactly what lets it corrupt silently.
A table loaded with an unknown partition transform isn't unpartitioned
(`IsUnpartitioned` only counts `VoidTransform`), so `WriteRecords` takes the
partitioned path, `getRecordPartitions` calls this `Apply`, gets `Valid:false`
for every row, and we write null partition values under a
`field=<transform-name>` dir — unreadable by Java/PyIceberg. On the `AddFiles`
path `fileToDataFile` panics instead.
I'd guard at `recordsToDataFiles`/`WriteRecords`: range the current spec and
reject if any field is an `UnknownTransform` before writing anything. That's
the spec rule too — writers must not commit against an unknown-transform spec.
Once that's in, this is good to land.
##########
table/update_spec.go:
##########
@@ -82,6 +82,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:
this went the opposite way from my round-1 ask, and I want to make sure
that's intentional rather than a slip.
Round 1 I asked to validate only newly-added fields, so a table with an
unknown transform on one column could still rename or drop an unrelated one.
This rejects any evolution up front — the `UpdateSpec` is dead before a single
op is queued.
If it's deliberate parity with Java's `BaseUpdatePartitionSpec`, I'm happy
with it — simpler, clearer signal, and I'll take Java's precedent over my
earlier ask. Just want to confirm it's the intended final direction. wdyt?
##########
transforms_test.go:
##########
@@ -103,6 +93,45 @@ func TestParseTransform(t *testing.T) {
assert.ErrorContains(t, err, tt.toparse)
})
}
+
+ // Unrecognized transform strings parse to an UnknownTransform (v3
requires
+ // readers to load unknown transforms) and round-trip verbatim,
preserving
+ // the original casing.
+ unknownTests := []struct {
+ name string
+ toparse string
+ }{
+ {"foobar", "foobar"},
+ {"bucket no brackets", "bucket"},
+ {"truncate no brackets", "truncate"},
+ {"bucket no val", "bucket[]"},
+ {"truncate no val", "truncate[]"},
+ {"bucket neg", "bucket[-1]"},
+ {"truncate neg", "truncate[-1]"},
+ {"bucket extra suffix", "bucketx[5]"},
+ {"bucket extra token", "bucket_extra[5]"},
+ {"truncate extra suffix", "truncatefoo[10]"},
+ {"truncate extra token", "truncate_garbage[4]"},
+ {"preserves original case", "Custom_V2[3]"},
+ }
+
+ for _, tt := range unknownTests {
+ t.Run("unknown/"+tt.name, func(t *testing.T) {
+ tr, err := iceberg.ParseTransform(tt.toparse)
+ require.NoError(t, err)
+ _, ok := tr.(iceberg.UnknownTransform)
+ assert.True(t, ok)
Review Comment:
tiny one: `assert.True(t, ok)` is non-fatal, so if `ParseTransform` ever
regresses and returns a non-`UnknownTransform`, execution falls through to the
bare `u := tr.(iceberg.UnknownTransform)` on line 130 and panics the goroutine
instead of failing cleanly. `require.True` here fixes it.
##########
table/metadata_internal_test.go:
##########
@@ -309,6 +309,31 @@ func TestMetadataV3Parsing(t *testing.T) {
assert.Equal(t, int64(2000), *secondSnapshot.FirstRowID)
}
+// A full v3 metadata document must load when its partition spec and sort order
+// use unknown transforms — nothing on the metadata path may trip a guard.
+func TestMetadataV3ParsesUnknownTransforms(t *testing.T) {
+ j := strings.NewReplacer(
+ `{"name": "x", "transform": "identity", "source-id": 1,
"field-id": 1000}`,
+ `{"name": "x", "transform": "custom_transform[42]",
"source-id": 1, "field-id": 1000}`,
+ `{"transform": "bucket[4]", "source-id": 3, "direction":
"desc", "null-order": "nulls-last"}`,
+ `{"transform": "custom_sort[7]", "source-id": 3, "direction":
"desc", "null-order": "nulls-last"}`,
+ ).Replace(ExampleTableMetadataV3)
+
+ meta, err := ParseMetadataBytes([]byte(j))
+ require.NoError(t, err)
+
+ spec := meta.PartitionSpec()
+ pf := spec.Field(0)
+ _, ok := pf.Transform.(iceberg.UnknownTransform)
+ assert.True(t, ok, "unknown partition transform should load")
+ assert.Equal(t, "custom_transform[42]", pf.Transform.String())
+
+ sf := meta.SortOrder().Field(1)
+ _, ok = sf.Transform.(iceberg.UnknownTransform)
+ assert.True(t, ok, "unknown sort transform should load")
+ assert.Equal(t, "custom_sort[7]", sf.Transform.String())
Review Comment:
this is the full-metadata round-trip I asked for last round, thanks — it
proves the load path doesn't trip a guard.
It only goes one direction though: `ParseMetadataBytes` then assert. The
field-level `TestPartitionFieldUnknownTransformRoundTrip` covers marshal-back,
so serialization itself is tested — but a marshal-back assert here (that the
output still contains `custom_transform[42]`/`custom_sort[7]`) is the exact
guardrail against `validateTransform` sneaking onto the write path that I was
after. Cheap to add while we're here.
--
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]
