laskoviymishka commented on code in PR #1834:
URL: https://github.com/apache/iceberg-go/pull/1834#discussion_r3844846334
##########
table/update_spec_test.go:
##########
@@ -551,11 +552,12 @@ func TestUpdateSpecBuildChanges(t *testing.T) {
assert.NotNil(t, reqs)
assert.Equal(t, 2, len(updates))
- assert.Equal(t, 1, len(reqs))
+ assert.Equal(t, 2, len(reqs))
assert.Equal(t, table.UpdateAddSpec, updates[0].Action())
assert.Equal(t, table.UpdateSetDefaultSpec, updates[1].Action())
assert.Equal(t, "assert-last-assigned-partition-id",
reqs[0].GetType())
+ assert.Equal(t,
table.AssertDefaultSpecID(testPartitionedTable.Metadata().DefaultPartitionSpec()),
reqs[1])
Review Comment:
These new assertions confirm the requirement is emitted with the right
value, but nothing exercises the actual fence: building the requirement from
base metadata, then calling `Validate` against metadata whose
`DefaultPartitionSpec()` has already advanced and expecting an error. That
`Validate` path is the property this PR exists to protect.
zeroshade flagged this as an optional nit; I'd actually want it before
merge, since a future edit that drops the append (or regresses
`assertDefaultSpecId.Validate`) wouldn't be caught otherwise. A subtest
mirroring `TestAssertLastAssignedPartitionIDValidate` would cover it. wdyt?
##########
table/update_spec.go:
##########
@@ -167,6 +167,11 @@ func (us *UpdateSpec) BuildUpdates() ([]Update,
[]Requirement, error) {
}
requiredLastAssignedPartitionId :=
us.txn.tbl.Metadata().LastPartitionSpecID()
requirements = append(requirements,
AssertLastAssignedPartitionID(*requiredLastAssignedPartitionId))
+ // Java UpdateRequirements.forUpdateTable registers
+ // AssertDefaultSpecID on SetDefaultPartitionSpec so two racing
+ // remove-only spec evolutions (which assign no new field ids)
+ // cannot both pass last-assigned-partition-id.
Review Comment:
The "cannot both pass" clause reads backwards. The whole reason for this
assertion is that two racing remove-only evolutions *can* both pass
`assert-last-assigned-partition-id` (they allocate no new field ids, so that
counter never moves), and the default-spec-id check is what makes the second
commit fail.
I'd flip it, something like:
```go
// Java UpdateRequirements.forUpdateTable registers
// AssertDefaultSpecID on SetDefaultPartitionSpec because two racing
// remove-only spec evolutions (which assign no new field ids) can
// both pass assert-last-assigned-partition-id; the default spec id
// check makes the second commit fail.
```
--
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]
