laskoviymishka commented on code in PR #1328:
URL: https://github.com/apache/iceberg-go/pull/1328#discussion_r3491560192
##########
table/transaction.go:
##########
@@ -142,6 +153,17 @@ func (t *Transaction) apply(updates []Update, reqs
[]Requirement) error {
return nil
}
+// requirementSemanticKey assumes Requirement JSON marshaling is canonical and
Review Comment:
More of a design question than a blocker: this comment names a load-bearing
invariant — every `Requirement` marshals to canonical, deterministic JSON —
that nothing enforces. It holds today because every requirement is flat scalars
in declaration order, but `Requirement` is exported, and a future field that's
a map (non-deterministic order) or one tagged `json:"-"` (two distinct
requirements collapsing to one key) would break the key silently.
Two ways to think about it. Keep `json.Marshal` but guard the invariant with
a test that round-trips each concrete requirement type twice and asserts equal
keys — cheap, and it turns the comment into something enforced. Or give
`Requirement` a typed `SemanticKey()` (e.g. `"assert-ref-snapshot-id:main:10"`)
so the equivalence contract is explicit rather than a side effect of marshaling
— more code, but it's the more honest abstraction and it'd sidestep the
same-ref question above too. Which direction feels right to you?
##########
table/transaction.go:
##########
@@ -114,12 +114,23 @@ func (t *Transaction) apply(updates []Update, reqs
[]Requirement) error {
existing := map[string]struct{}{}
for _, r := range t.reqs {
- existing[r.GetType()] = struct{}{}
+ key, err := requirementSemanticKey(r)
+ if err != nil {
+ return err
+ }
+
+ existing[key] = struct{}{}
}
for _, r := range reqs {
- if _, ok := existing[r.GetType()]; !ok {
+ key, err := requirementSemanticKey(r)
+ if err != nil {
+ return err
+ }
+
+ if _, ok := existing[key]; !ok {
Review Comment:
Now that the key is the full payload, I'd like to think through one case
with you. If `apply` runs more than once on the same `Transaction` for the same
ref at different snapshot IDs — say an append on an empty table emits
`AssertRefSnapshotID("main", nil)`, then a later append emits
`AssertRefSnapshotID("main", &firstID)` — those serialize to different JSON
(`"snapshot-id":null` vs `:<id>`), so both survive dedupe and both go to the
catalog. The base state only matches one of them, so I'd expect the other to
409. The old type-only key collapsed them into one.
Is that path reachable in practice, or does `apply` only ever see one
assertion per ref per transaction? If it's reachable, the design question is
what the key should really be: full payload preserves the genuinely-distinct
refs you're fixing, but it also preserves same-ref/different-id pairs that we
probably don't want both of. Java threads this with a per-ref `changedRefs` set
— first assertion per ref wins. One option here is to key `assertRefSnapshotID`
on type + ref and fall back to payload for the rest. How are you thinking about
it?
--
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]
