laskoviymishka commented on code in PR #1370:
URL: https://github.com/apache/iceberg-go/pull/1370#discussion_r3539727946
##########
table/transaction.go:
##########
@@ -153,9 +153,25 @@ func (t *Transaction) apply(updates []Update, reqs
[]Requirement) error {
return nil
}
-// requirementSemanticKey assumes Requirement JSON marshaling is canonical and
-// deterministic for every requirement type that participates in dedupe.
+// requirementSemanticKey computes the dedupe key for a requirement.
+//
+// For most requirement types the key is the canonical JSON marshaling, which
+// keeps semantically distinct requirements (e.g. assertions on different refs)
+// from collapsing into one another.
+//
+// assert-ref-snapshot-id is special-cased to key by requirement type + ref
name
+// only, deliberately ignoring the asserted snapshot id. Within a single
+// transaction the builder mutates its own ref state across operations (e.g.
the
+// first append asserts main == nil, a later append asserts main ==
snapshot-1),
+// which would otherwise produce multiple, mutually contradictory base-state
+// assertions for the same ref against the pre-transaction metadata. Keying by
+// ref name keeps only the first assertion for each ref while still letting
+// assertions for different refs survive dedupe.
func requirementSemanticKey(r Requirement) (string, error) {
+ if ref, ok := r.(*assertRefSnapshotID); ok {
+ return fmt.Sprintf("%s\x00%s", reqAssertRefSnapshotID,
ref.Ref), nil
Review Comment:
Tiny one: this can just be `reqAssertRefSnapshotID + "\x00" + ref.Ref` —
perfsprint won't love a `Sprintf` that's really string concatenation, and it's
a hair faster.
While we're here, a half-line noting `\x00` is a safe separator because
Iceberg ref names can't contain NUL would save the next person a double-take.
##########
table/transaction.go:
##########
@@ -153,9 +153,25 @@ func (t *Transaction) apply(updates []Update, reqs
[]Requirement) error {
return nil
}
-// requirementSemanticKey assumes Requirement JSON marshaling is canonical and
-// deterministic for every requirement type that participates in dedupe.
+// requirementSemanticKey computes the dedupe key for a requirement.
+//
+// For most requirement types the key is the canonical JSON marshaling, which
+// keeps semantically distinct requirements (e.g. assertions on different refs)
+// from collapsing into one another.
+//
+// assert-ref-snapshot-id is special-cased to key by requirement type + ref
name
+// only, deliberately ignoring the asserted snapshot id. Within a single
+// transaction the builder mutates its own ref state across operations (e.g.
the
+// first append asserts main == nil, a later append asserts main ==
snapshot-1),
+// which would otherwise produce multiple, mutually contradictory base-state
+// assertions for the same ref against the pre-transaction metadata. Keying by
+// ref name keeps only the first assertion for each ref while still letting
+// assertions for different refs survive dedupe.
func requirementSemanticKey(r Requirement) (string, error) {
+ if ref, ok := r.(*assertRefSnapshotID); ok {
Review Comment:
The whole first-wins-is-base-state property only holds because `commit()`
captures the builder's ref state before `apply()` applies that op's updates —
so the first assertion we see for a ref is the pre-transaction one. That's true
today, but it's an implicit ordering contract, not something the types enforce.
I'd add a line here (and near the `commit()` call site in
`snapshot_producers.go`) spelling out that dedupe keeps the first assertion
*because* it's the base-state one, and that this breaks silently — wrong
precondition to the catalog, no error — if a future refactor ever advances the
builder before `commit()` runs. Cheap insurance. wdyt?
##########
table/transaction_internal_test.go:
##########
@@ -60,6 +60,116 @@ func
TestTransactionApplyDedupesEquivalentRequirementsWithinAndAcrossCalls(t *te
requireContainsRefSnapshotRequirement(t, txn.reqs, MainBranch,
&mainSnapshotID)
}
+// TestTransactionApplyDedupesSameRefAssertionsNewTable covers two appends in a
+// single new-table transaction: the first asserts main must not exist, and the
+// second (after the builder has created main) asserts main == the new
snapshot.
+// Only the first main == nil assertion, which reflects the pre-transaction
base
+// state, must be retained.
+func TestTransactionApplyDedupesSameRefAssertionsNewTable(t *testing.T) {
+ txn, _ := createTestTransactionWithMemIO(t, *iceberg.UnpartitionedSpec)
+
+ // First append: main does not exist yet in the pre-transaction
metadata.
+ err := txn.apply(nil, []Requirement{AssertRefSnapshotID(MainBranch,
nil)})
+ require.NoError(t, err)
+ require.Len(t, txn.reqs, 1)
+ requireContainsRefSnapshotRequirement(t, txn.reqs, MainBranch, nil)
+
+ // Simulate the first append creating main -> 10 in the transaction
builder.
+ require.NoError(t, txn.meta.AddSnapshot(&Snapshot{
+ SnapshotID: 10,
+ SequenceNumber: 1,
+ ManifestList:
"mem://default/table-location/metadata/manifest-10.avro",
+ Summary: &Summary{Operation: OpAppend},
+ TimestampMs: time.Now().UnixMilli(),
+ }))
+ require.NoError(t, txn.meta.SetSnapshotRef(MainBranch, 10, BranchRef))
+
+ // Second append asserts main == 10; it must dedupe against the first
+ // assertion for main rather than adding a contradictory base-state
check.
+ newHead := int64(10)
+ err = txn.apply(nil, []Requirement{AssertRefSnapshotID(MainBranch,
&newHead)})
Review Comment:
Optional: every one of these uses two separate `apply()` calls, so the
cross-call dedupe path is well covered but the within-a-single-`apply()` case —
same ref twice in one reqs slice, different snapshot ids — isn't. The loop
handles it today, but a future "build a set then append" refactor would pass
all these tests while quietly breaking the within-call case. A quick variant of
this test with both assertions in one `apply()` would close that.
--
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]
