zeroshade commented on code in PR #1651:
URL: https://github.com/apache/iceberg-go/pull/1651#discussion_r3732110893
##########
table/update_spec.go:
##########
@@ -145,14 +154,14 @@ func (us *UpdateSpec) BuildUpdates() ([]Update,
[]Requirement, error) {
updates := make([]Update, 0)
requirements := make([]Requirement, 0)
- if us.txn.tbl.Metadata().DefaultPartitionSpec() != newSpec.ID() {
+ if us.meta.DefaultPartitionSpec() != newSpec.ID() {
if us.isNewPartitionSpec(newSpec.ID()) {
updates = append(updates,
NewAddPartitionSpecUpdate(&newSpec, false))
updates = append(updates, NewSetDefaultSpecUpdate(-1))
} else {
updates = append(updates,
NewSetDefaultSpecUpdate(newSpec.ID()))
}
- requiredLastAssignedPartitionId :=
us.txn.tbl.Metadata().LastPartitionSpecID()
+ requiredLastAssignedPartitionId := us.meta.LastPartitionSpecID()
Review Comment:
On the second staged spec update, `us.meta` contains the first update's last
partition-field ID, while the first update already staged a requirement for the
original value. Both requirements survive deduplication and are validated
against the same catalog snapshot, so they contradict each other and cannot be
repaired by retry. **Suggested fix:** derive this requirement from
`us.txn.tbl.Metadata()` so every staged update asserts the same original/base
value, or make this a singleton requirement that retains the first value.
##########
table/update_spec_test.go:
##########
@@ -280,6 +280,110 @@ func TestUpdateSpecAddField(t *testing.T) {
})
}
+func TestUpdateSpecReadsStagedTransactionMetadata(t *testing.T) {
+ t.Run("end-to-end: partition by column added earlier in the same
transaction", func(t *testing.T) {
+ txn := testNonPartitionedTable.NewTransaction()
+
+ require.NoError(t, txn.UpdateSchema(false, false).
+ AddColumn([]string{"new_col"},
iceberg.PrimitiveTypes.String, "", false, nil).
+ Commit())
+
+ require.NoError(t, txn.UpdateSpec(false).
+ AddField("new_col", iceberg.IdentityTransform{},
"new_col_identity").
+ Commit())
+
+ stagedTbl, err := txn.StagedTable()
+ require.NoError(t, err)
+
+ // The new column is assigned schema field id 8 (the existing
schema
+ // occupies ids 1-7), so the partition field must reference
source id 8.
+ spec := stagedTbl.Spec()
+ added := spec.FieldsBySourceID(8)
+ require.Len(t, added, 1)
+ assert.Equal(t, "new_col_identity", added[0].Name)
+ assert.Equal(t, iceberg.IdentityTransform{}, added[0].Transform)
+ assert.Equal(t, iceberg.PartitionDataIDStart, added[0].FieldID)
+ })
+
+ t.Run("auto-generated partition name resolves against the staged
schema", func(t *testing.T) {
+ txn := testNonPartitionedTable.NewTransaction()
+
+ require.NoError(t, txn.UpdateSchema(false, false).
+ AddColumn([]string{"new_col"},
iceberg.PrimitiveTypes.String, "", false, nil).
+ Commit())
+
+ // An empty target name forces GeneratePartitionFieldName,
which must
+ // resolve the source column against the staged schema.
+ specUpdate := txn.UpdateSpec(false)
+ _, _, err := specUpdate.
+ AddField("new_col", iceberg.IdentityTransform{}, "").
+ BuildUpdates()
+ require.NoError(t, err)
+
+ newSpec, err := specUpdate.Apply()
+ require.NoError(t, err)
+ added := newSpec.FieldsBySourceID(8)
+ require.Len(t, added, 1)
+ assert.Equal(t, "new_col", added[0].Name)
+ })
+
+ t.Run("end-to-end: chained UpdateSpec sees partition fields staged
earlier", func(t *testing.T) {
+ txn := testNonPartitionedTable.NewTransaction()
+
+ // Two independent UpdateSpec commits in the same transaction.
The
+ // second must observe the field staged by the first.
+ require.NoError(t,
txn.UpdateSpec(false).AddIdentity("id").Commit())
+ require.NoError(t,
txn.UpdateSpec(false).AddIdentity("name").Commit())
+
+ stagedTbl, err := txn.StagedTable()
Review Comment:
This stops at `StagedTable()`, before the contradictory requirements are
sent to the catalog. **Suggested fix:** follow the two staged updates with
`txn.Commit(ctx)`, assert that both fields are committed with exactly one base
`last-assigned-partition-id` requirement, and cover the retry path as well.
--
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]
