laskoviymishka commented on code in PR #1873:
URL: https://github.com/apache/iceberg-go/pull/1873#discussion_r3850555929
##########
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"`
initial bool
}
// NewAddPartitionSpecUpdate creates a new update that adds the given
partition spec to the table
// metadata. If the initial flag is set to true, the spec is considered the
initial spec of the table,
// and all other previously added specs in the metadata builder are removed.
func NewAddPartitionSpecUpdate(spec *iceberg.PartitionSpec, initial bool)
*addPartitionSpecUpdate {
- return &addPartitionSpecUpdate{
+ upd := &addPartitionSpecUpdate{
baseUpdate: baseUpdate{ActionName: UpdateAddSpec},
- Spec: spec,
initial: initial,
}
+ if spec != nil {
+ upd.Spec = &iceberg.UnboundPartitionSpec{PartitionSpec: *spec}
+ }
+
+ return upd
}
func (u *addPartitionSpecUpdate) Apply(builder *MetadataBuilder) error {
- return builder.AddPartitionSpec(u.Spec, u.initial)
+ if u.Spec == nil {
+ return fmt.Errorf("%w: %s requires field %q",
iceberg.ErrInvalidArgument, UpdateAddSpec, "spec")
Review Comment:
These nil guards format the action name bare, but every other "requires
field" error in this file uses the `validateRequiredUpdateFields` template
`"%w: update %q requires field %q"`. So this path emits `add-spec requires
field "spec"` while the decode path emits `update "add-spec" requires field
"spec"`. Same divergence at line 466 for sort-order. I'd match the existing
template so the two paths read identically.
##########
table/updates.go:
##########
@@ -428,22 +440,33 @@ func (u *setDefaultSpecUpdate) Apply(builder
*MetadataBuilder) error {
type addSortOrderUpdate struct {
baseUpdate
- SortOrder *SortOrder `json:"sort-order"`
+ // Unbound for the same reason as addPartitionSpecUpdate.Spec: Apply
checks
Review Comment:
The symmetry with the partition-spec fix only holds at decode time. For
specs the fix is end-to-end, but `SortOrder.CheckCompatibility` still calls
`validateSortSourceIDs` unconditionally and rejects a void sort tombstone with
"source ID must be positive: 0" at Apply.
So a void sort tombstone now decodes fine but still fails during Apply,
which isn't obvious from a comment that implies parity. I'd either carry the
void exception through `CheckCompatibility` too, or soften the comment to say
we're only deferring the error here, not accepting the tombstone. wdyt?
##########
table/updates_test.go:
##########
@@ -524,12 +524,12 @@ func TestUnmarshalUpdates(t *testing.T) {
case "add-partition-spec":
expectedAddPartitionSpec :=
u.(*addPartitionSpecUpdate)
actualAddPartitionSpec :=
actual[idx].(*addPartitionSpecUpdate)
- assert.True(t,
expectedAddPartitionSpec.Spec.Equals(*actualAddPartitionSpec.Spec))
+ assert.True(t,
expectedAddPartitionSpec.Spec.Equals(actualAddPartitionSpec.Spec.PartitionSpec))
Review Comment:
This is the assertion the PR just updated, but it never runs. `Action()`
returns `add-spec` (`UpdateAddSpec`), and the case label above is
`"add-partition-spec"`, so the whole branch falls through to `default` and the
spec is never compared.
Even if the label matched, `expectedAddPartitionSpec` and
`actualAddPartitionSpec` are both cast from `actual[idx]` (`u` *is*
`actual[idx]` in this range loop), so it compares the decoded spec against
itself and passes vacuously. The `add-sort-order` branch below has the same
self-comparison shape.
I'd switch to `case UpdateAddSpec:` and compare `tc.expected[idx]` against
`actual[idx]` so the round-trip is genuinely exercised. This is the one thing
I'd want fixed before merge, since it's the coverage this change is really
adding.
##########
table/updates_test.go:
##########
@@ -1299,3 +1299,62 @@ func TestRemoveEncryptionKeyUpdate_Apply_NoOp(t
*testing.T) {
b := buildFromBase(t)
require.NoError(t, NewRemoveEncryptionKeyUpdate("nonexistent").Apply(b))
}
+
+func TestAddPartitionSpecUpdate_UnmarshalVoidTombstone(t *testing.T) {
Review Comment:
Not this PR's job to solve, but worth flagging while we're here: this now
accepts and persists a partition field with source-id 0, and
`PartitionField.MarshalJSON` omits `source-id` when it's 0. Java's
`PartitionSpecParser` calls `JsonUtil.getInt("source-id", ...)`
unconditionally, so metadata written with such a tombstone won't parse back in
Java or PyIceberg.
The create-table path already had this exposure, so this PR only widens the
set of inputs that reach it. I wouldn't block on it, but a tracking issue for
canonicalizing the void tombstone (Java preserves the original positive
source-id rather than storing 0) would be good so we don't lose it.
##########
catalog/rest/load_table_bench_test.go:
##########
@@ -98,7 +98,7 @@ func makeTableResponseWithSnapshots(snapshotCount int64)
[]byte {
schemaID := 0
var snapshotTimestamp int64
var snapshotID int64
- for i := int64(0); i < snapshotCount; i++ {
+ for i := range snapshotCount {
Review Comment:
This `for range` change is unrelated to the decode fix, so I'd pull it into
its own commit to keep the bug fix easy to bisect.
It's also not purely cosmetic: in the old loop `i` was shared across
iterations, so `parentID = &i` captured one address and every snapshot ended up
pointing at the final count. Range-over-int gives each iteration its own `i`,
which quietly corrects that. Fine to keep, but worth calling out as a behavior
change rather than folding in as style.
##########
table/updates_test.go:
##########
@@ -1299,3 +1299,62 @@ 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.NotContains(t, err.Error(), "must be positive")
Review Comment:
This pins the absence of the old decode error but nothing about the new one,
so a future regression that fails for a different reason would slip through.
The sort-order test right below asserts the positive message ("not compatible
with current schema"), so I'd add the analogous `assert.Contains` on the
expected binding error here too.
--
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]
