laskoviymishka commented on code in PR #1873:
URL: https://github.com/apache/iceberg-go/pull/1873#discussion_r3862376310
##########
table/updates.go:
##########
@@ -389,23 +389,35 @@ func (u *setCurrentSchemaUpdate) Apply(builder
*MetadataBuilder) error {
type addPartitionSpecUpdate struct {
baseUpdate
- Spec *iceberg.PartitionSpec `json:"spec"`
+ // The spec arrives unbound: Apply binds it to the table's current
schema,
+ // so its source IDs are only required to resolve there. A dropped
+ // partition field in particular is a void transform over source ID 0,
+ // which BindToSchema carries across but a bound decode rejects.
+ Spec *iceberg.UnboundPartitionSpec `json:"spec"`
Review Comment:
The behavior here is right, and it matches how Java stores this
(`AddPartitionSpec` holds an `UnboundPartitionSpec` too), so I'm not worried
about correctness.
What gives me pause is that `UnboundPartitionSpec`'s own doc says to use
`PartitionSpec` for specs read from table metadata, where source IDs are bound
and must be positive — and an add-spec commit payload is exactly that. So we're
using the type against its documented contract, and the next person who reads
that doc hits a trap: the bound positivity check that would catch a genuinely
bad source ID is now bypassed at decode time.
Two ways out, and either is fine as long as we're explicit: keep
`*UnboundPartitionSpec` here and loosen that doc comment to acknowledge the
commit path, or add a void-tombstone exemption to the bound decoder (parallel
to `BindToSchema`) and keep `*iceberg.PartitionSpec` — the new tests would
drive the same behavior either way. I lean slightly toward the second since it
keeps the type contract honest, but I don't feel strongly. wdyt?
##########
table/updates.go:
##########
@@ -428,22 +440,33 @@ func (u *setDefaultSpecUpdate) Apply(builder
*MetadataBuilder) error {
type addSortOrderUpdate struct {
baseUpdate
- SortOrder *SortOrder `json:"sort-order"`
+ // Unbound so decoding accepts placeholder source IDs. Unlike the spec
+ // above, none are exempt at Apply: CheckCompatibility still requires
every
+ // source ID to resolve in the current schema, so this only defers the
error.
+ SortOrder *UnboundSortOrder `json:"sort-order"`
initial bool
}
// NewAddSortOrderUpdate creates a new update that adds the given sort order
to the table metadata.
// If the initial flag is set to true, the sort order is considered the
initial sort order of the table,
// and all previously added sort orders in the metadata builder are removed.
func NewAddSortOrderUpdate(sortOrder *SortOrder) *addSortOrderUpdate {
- return &addSortOrderUpdate{
+ upd := &addSortOrderUpdate{
baseUpdate: baseUpdate{ActionName: UpdateAddSortOrder},
- SortOrder: sortOrder,
}
+ if sortOrder != nil {
+ upd.SortOrder = &UnboundSortOrder{SortOrder: *sortOrder}
+ }
+
+ return upd
}
func (u *addSortOrderUpdate) Apply(builder *MetadataBuilder) error {
- return builder.AddSortOrder(u.SortOrder)
+ if u.SortOrder == nil {
+ return fmt.Errorf("%w: update %q requires field %q",
iceberg.ErrInvalidArgument, UpdateAddSortOrder, "sort-order")
+ }
+
+ return builder.AddSortOrder(&u.SortOrder.SortOrder)
Review Comment:
Minor lifetime thing: `AddSortOrder` stashes `&sortOrder.orderID` in
`b.lastAddedSortOrderID` and holds it for the builder's lifetime, so after this
call the builder is pointing into `u.SortOrder.SortOrder` — a field of this
update struct. It's fine as long as the `Updates` slice outlives the builder,
which is the normal pattern, but nothing enforces or documents it.
A one-line comment on this call noting the builder aliases the embedded
order would save the next person the trace. Not blocking.
##########
table/updates_test.go:
##########
@@ -1299,3 +1299,63 @@ 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))
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "cannot find source column with id: 0
in schema")
Review Comment:
The `NotContains(..., "must be positive")` below is the load-bearing
assertion here — it's what proves the rejection moved from decode-time to
binding — but the `Contains` on `"cannot find source column with id: 0 in
schema"` pins us to a phrase from deep inside `BindToSchema`. If that wording
ever changes, the `require.Error` above still passes and the failure shows up
as a bare `false`.
I'd switch it to `require.ErrorContains` so a regression stops with a useful
message, and drop a one-line comment that we're intentionally pinning the error
source to binding. Small thing.
##########
table/updates_test.go:
##########
@@ -1299,3 +1299,63 @@ 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))
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "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)
+
+ err := updates[0].Apply(buildFromBaseV3(t))
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "not compatible with current schema")
Review Comment:
This one only ever exercises the failure path — decode succeeds, Apply
rejects. We've got no test that a resolvable add-sort-order actually
round-trips: decodes as `*UnboundSortOrder`, applies, builds, and comes out
with the right direction/null-order/transform.
That's the half that actually changed here — the decode now goes through
`newSortOrder(..., false)`, which skips source-ID validation — and right now a
parsing regression in it would only be caught by `TestUnmarshalUpdates`, which
never reaches Apply+Build.
Mirror the void-tombstone test with a source ID that resolves in
`baseMetaJSON` (field 1, `x`), apply it, build, and assert the field comes back
correct. Then both sides of the new construction are covered.
--
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]
