laskoviymishka commented on code in PR #1873:
URL: https://github.com/apache/iceberg-go/pull/1873#discussion_r3872854925
##########
table/updates_test.go:
##########
@@ -1299,3 +1299,94 @@ func TestRemoveEncryptionKeyUpdate_Apply_NoOp(t
*testing.T) {
b := buildFromBase(t)
require.NoError(t, NewRemoveEncryptionKeyUpdate("nonexistent").Apply(b))
}
+
+func TestAddPartitionSpecUpdate_UnmarshalVoidTombstone(t *testing.T) {
+ // A dropped partition field whose source column is gone is a void
+ // transform over source ID 0; BindToSchema carries it across, so the
+ // decoder must let it through.
+ data :=
[]byte(`[{"action":"add-spec","spec":{"spec-id":1,"fields":[{"source-id":0,"field-id":1000,"transform":"void","name":"x_bucket"}]}}]`)
+
+ var updates Updates
+ require.NoError(t, json.Unmarshal(data, &updates))
+ require.Len(t, updates, 1)
+
+ b := buildFromBaseV3(t)
+ require.NoError(t, updates[0].Apply(b))
+
+ meta, err := b.Build()
Review Comment:
This test is the first place a void tombstone rides all the way through
decode, bind, and Build, which is exactly the path we want covered, but it
stops at Build and never marshals `meta` back out, and that's where I think
there's a real interop hazard.
`PartitionField.MarshalJSON` special-cases void tombstones and omits
`source-id`, so the spec we persist here serializes with no `source-id` key at
all. Java's `PartitionSpecParser.fromJson` and PyIceberg both require
`source-id`, so once iceberg-go writes this file neither can read it back.
Before this PR the void path was rejected at decode, so that marshal branch
never actually ran, which is why it's only surfacing now.
I think the fix is to drop the void special-case in `MarshalJSON`, since the
normal path already writes `source-id: 0` and matches Java, and a
`json.Marshal` + `assert.JSONEq` round-trip on this fixture would both pin the
behavior and have caught this. If you'd rather do the marshal fix as a
follow-up with an issue that's fine, but I'd want it decided before merge. wdyt?
##########
table/updates_test.go:
##########
@@ -1299,3 +1299,94 @@ func TestRemoveEncryptionKeyUpdate_Apply_NoOp(t
*testing.T) {
b := buildFromBase(t)
require.NoError(t, NewRemoveEncryptionKeyUpdate("nonexistent").Apply(b))
}
+
+func TestAddPartitionSpecUpdate_UnmarshalVoidTombstone(t *testing.T) {
+ // A dropped partition field whose source column is gone is a void
+ // transform over source ID 0; BindToSchema carries it across, so the
+ // decoder must let it through.
+ data :=
[]byte(`[{"action":"add-spec","spec":{"spec-id":1,"fields":[{"source-id":0,"field-id":1000,"transform":"void","name":"x_bucket"}]}}]`)
+
+ var updates Updates
+ require.NoError(t, json.Unmarshal(data, &updates))
+ require.Len(t, updates, 1)
+
+ b := buildFromBaseV3(t)
+ require.NoError(t, updates[0].Apply(b))
+
+ meta, err := b.Build()
+ require.NoError(t, err)
+
+ spec := meta.PartitionSpecByID(1)
+ require.NotNil(t, spec)
+ require.Equal(t, 1, spec.NumFields())
+ assert.Equal(t, "x_bucket", spec.Field(0).Name)
+ assert.IsType(t, iceberg.VoidTransform{}, spec.Field(0).Transform)
+}
+
+func TestAddPartitionSpecUpdate_UnmarshalUnresolvableSourceID(t *testing.T) {
+ // A non-void source ID that the current schema cannot resolve is still
+ // rejected, but by binding rather than by decoding.
+ data :=
[]byte(`[{"action":"add-spec","spec":{"spec-id":1,"fields":[{"source-id":0,"field-id":1000,"transform":"identity","name":"x"}]}}]`)
+
+ var updates Updates
+ require.NoError(t, json.Unmarshal(data, &updates))
+ require.Len(t, updates, 1)
+
+ err := updates[0].Apply(buildFromBaseV3(t))
+ // Pinned to BindToSchema's wording on purpose: the assertion below only
+ // shows the decoder let the field through, not who rejected it.
+ require.ErrorContains(t, err, "cannot find source column with id: 0 in
schema")
+ assert.NotContains(t, err.Error(), "must be positive")
+}
+
+func TestAddSortOrderUpdate_UnmarshalDefersBindingToApply(t *testing.T) {
+ // Decoding accepts the order; the schema it must resolve against is
only
+ // known at Apply, which is where an unresolvable source ID is reported.
+ data :=
[]byte(`[{"action":"add-sort-order","sort-order":{"order-id":1,"fields":[{"source-id":0,"transform":"identity","direction":"asc","null-order":"nulls-first"}]}}]`)
Review Comment:
source-id 0 here gets rejected by `validateSortSourceIDs` ("source ID must
be positive: 0") before any schema lookup runs, so this proves positive-ID
enforcement moved to Apply rather than the schema-resolution deferral the name
and comment describe. The `FindFieldByID` branch never executes.
If we use a positive source-id that isn't in the V3 base schema (say 999),
Apply fails with "sort field with source id 999 not found in schema" and the
test actually exercises the deferred schema lookup. wdyt?
##########
table/updates_test.go:
##########
@@ -1299,3 +1299,94 @@ func TestRemoveEncryptionKeyUpdate_Apply_NoOp(t
*testing.T) {
b := buildFromBase(t)
require.NoError(t, NewRemoveEncryptionKeyUpdate("nonexistent").Apply(b))
}
+
+func TestAddPartitionSpecUpdate_UnmarshalVoidTombstone(t *testing.T) {
+ // A dropped partition field whose source column is gone is a void
+ // transform over source ID 0; BindToSchema carries it across, so the
+ // decoder must let it through.
+ data :=
[]byte(`[{"action":"add-spec","spec":{"spec-id":1,"fields":[{"source-id":0,"field-id":1000,"transform":"void","name":"x_bucket"}]}}]`)
+
+ var updates Updates
+ require.NoError(t, json.Unmarshal(data, &updates))
+ require.Len(t, updates, 1)
+
+ b := buildFromBaseV3(t)
+ require.NoError(t, updates[0].Apply(b))
+
+ meta, err := b.Build()
+ require.NoError(t, err)
+
+ spec := meta.PartitionSpecByID(1)
+ require.NotNil(t, spec)
+ require.Equal(t, 1, spec.NumFields())
+ assert.Equal(t, "x_bucket", spec.Field(0).Name)
+ assert.IsType(t, iceberg.VoidTransform{}, spec.Field(0).Transform)
+}
+
+func TestAddPartitionSpecUpdate_UnmarshalUnresolvableSourceID(t *testing.T) {
+ // A non-void source ID that the current schema cannot resolve is still
+ // rejected, but by binding rather than by decoding.
+ data :=
[]byte(`[{"action":"add-spec","spec":{"spec-id":1,"fields":[{"source-id":0,"field-id":1000,"transform":"identity","name":"x"}]}}]`)
+
+ var updates Updates
+ require.NoError(t, json.Unmarshal(data, &updates))
+ require.Len(t, updates, 1)
+
+ err := updates[0].Apply(buildFromBaseV3(t))
+ // Pinned to BindToSchema's wording on purpose: the assertion below only
+ // shows the decoder let the field through, not who rejected it.
+ require.ErrorContains(t, err, "cannot find source column with id: 0 in
schema")
+ assert.NotContains(t, err.Error(), "must be positive")
+}
+
+func TestAddSortOrderUpdate_UnmarshalDefersBindingToApply(t *testing.T) {
+ // Decoding accepts the order; the schema it must resolve against is
only
+ // known at Apply, which is where an unresolvable source ID is reported.
+ data :=
[]byte(`[{"action":"add-sort-order","sort-order":{"order-id":1,"fields":[{"source-id":0,"transform":"identity","direction":"asc","null-order":"nulls-first"}]}}]`)
+
+ var updates Updates
+ require.NoError(t, json.Unmarshal(data, &updates))
+ require.Len(t, updates, 1)
+
+ require.ErrorContains(t, updates[0].Apply(buildFromBaseV3(t)), "not
compatible with current schema")
+}
+
+func TestAddSortOrderUpdate_UnmarshalRoundTrip(t *testing.T) {
+ // The decode no longer validates source IDs, so cover the resolvable
case
+ // end to end: field 1 is x in baseMetaV3JSON.
+ data :=
[]byte(`[{"action":"add-sort-order","sort-order":{"order-id":1,"fields":[{"source-id":1,"transform":"identity","direction":"desc","null-order":"nulls-last"}]}}]`)
+
+ var updates Updates
+ require.NoError(t, json.Unmarshal(data, &updates))
+ require.Len(t, updates, 1)
+
+ b := buildFromBaseV3(t)
+ require.NoError(t, updates[0].Apply(b))
+
+ meta, err := b.Build()
+ require.NoError(t, err)
+
+ var order SortOrder
+ var found bool
+ for _, o := range meta.SortOrders() {
+ if o.OrderID() == 1 {
+ order, found = o, true
+ }
+ }
+ require.True(t, found)
+ require.Equal(t, 1, order.Len())
+
+ field := order.Field(0)
+ assert.Equal(t, []int{1}, field.SourceIDs)
+ assert.Equal(t, SortDESC, field.Direction)
+ assert.Equal(t, NullsLast, field.NullOrder)
+ assert.IsType(t, iceberg.IdentityTransform{}, field.Transform)
+}
+
+func TestAddSpecAndSortOrderUpdates_ApplyRejectNilPayload(t *testing.T) {
Review Comment:
These two nil-payload checks run sequentially with `require`, so if the spec
assertion fails the sort-order one never runs. I'd split them into two `t.Run`
subtests so both always execute and you can see which one broke.
##########
table/updates_test.go:
##########
@@ -1299,3 +1299,94 @@ func TestRemoveEncryptionKeyUpdate_Apply_NoOp(t
*testing.T) {
b := buildFromBase(t)
require.NoError(t, NewRemoveEncryptionKeyUpdate("nonexistent").Apply(b))
}
+
+func TestAddPartitionSpecUpdate_UnmarshalVoidTombstone(t *testing.T) {
+ // A dropped partition field whose source column is gone is a void
+ // transform over source ID 0; BindToSchema carries it across, so the
+ // decoder must let it through.
+ data :=
[]byte(`[{"action":"add-spec","spec":{"spec-id":1,"fields":[{"source-id":0,"field-id":1000,"transform":"void","name":"x_bucket"}]}}]`)
+
+ var updates Updates
+ require.NoError(t, json.Unmarshal(data, &updates))
+ require.Len(t, updates, 1)
+
+ b := buildFromBaseV3(t)
+ require.NoError(t, updates[0].Apply(b))
+
+ meta, err := b.Build()
+ require.NoError(t, err)
+
+ spec := meta.PartitionSpecByID(1)
+ require.NotNil(t, spec)
+ require.Equal(t, 1, spec.NumFields())
+ assert.Equal(t, "x_bucket", spec.Field(0).Name)
+ assert.IsType(t, iceberg.VoidTransform{}, spec.Field(0).Transform)
+}
+
+func TestAddPartitionSpecUpdate_UnmarshalUnresolvableSourceID(t *testing.T) {
+ // A non-void source ID that the current schema cannot resolve is still
+ // rejected, but by binding rather than by decoding.
+ data :=
[]byte(`[{"action":"add-spec","spec":{"spec-id":1,"fields":[{"source-id":0,"field-id":1000,"transform":"identity","name":"x"}]}}]`)
+
+ var updates Updates
+ require.NoError(t, json.Unmarshal(data, &updates))
+ require.Len(t, updates, 1)
+
+ err := updates[0].Apply(buildFromBaseV3(t))
+ // Pinned to BindToSchema's wording on purpose: the assertion below only
+ // shows the decoder let the field through, not who rejected it.
+ require.ErrorContains(t, err, "cannot find source column with id: 0 in
schema")
+ assert.NotContains(t, err.Error(), "must be positive")
+}
+
+func TestAddSortOrderUpdate_UnmarshalDefersBindingToApply(t *testing.T) {
+ // Decoding accepts the order; the schema it must resolve against is
only
+ // known at Apply, which is where an unresolvable source ID is reported.
+ data :=
[]byte(`[{"action":"add-sort-order","sort-order":{"order-id":1,"fields":[{"source-id":0,"transform":"identity","direction":"asc","null-order":"nulls-first"}]}}]`)
+
+ var updates Updates
+ require.NoError(t, json.Unmarshal(data, &updates))
+ require.Len(t, updates, 1)
+
+ require.ErrorContains(t, updates[0].Apply(buildFromBaseV3(t)), "not
compatible with current schema")
+}
+
+func TestAddSortOrderUpdate_UnmarshalRoundTrip(t *testing.T) {
+ // The decode no longer validates source IDs, so cover the resolvable
case
+ // end to end: field 1 is x in baseMetaV3JSON.
+ data :=
[]byte(`[{"action":"add-sort-order","sort-order":{"order-id":1,"fields":[{"source-id":1,"transform":"identity","direction":"desc","null-order":"nulls-last"}]}}]`)
+
+ var updates Updates
+ require.NoError(t, json.Unmarshal(data, &updates))
+ require.Len(t, updates, 1)
+
+ b := buildFromBaseV3(t)
+ require.NoError(t, updates[0].Apply(b))
+
+ meta, err := b.Build()
+ require.NoError(t, err)
+
+ var order SortOrder
+ var found bool
+ for _, o := range meta.SortOrders() {
+ if o.OrderID() == 1 {
+ order, found = o, true
Review Comment:
Small one: this keeps scanning after it finds order 1, so if a future
fixture ever produced two matching orders the last would silently win. A
`break` after the assignment makes the intent clear.
--
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]
