zeroshade commented on code in PR #1955:
URL: https://github.com/apache/iceberg-go/pull/1955#discussion_r3936148243
##########
table/transaction.go:
##########
@@ -172,47 +172,61 @@ func (t *Transaction) apply(updates []Update, reqs
[]Requirement) error {
return errors.New("cannot apply updates to nil metadata")
}
- current, err := stagedMeta.Build()
- if err != nil {
- return err
- }
+ // Only new requirement validation needs the immutable metadata view.
+ // Updates can be applied directly to the staged builder, and duplicate
Review Comment:
**minor** — StagedTable() metadata-log output changed; PR body claims
publish behavior is unchanged
Removing the eager Build() also removes its non-idempotent AppendMetadataLog
side effect (table/metadata.go:1378-1384). Three SetProperties calls previously
yielded three duplicate previous-file entries from StagedTable(); now they
yield one. This is a fix, but it is an observable output change that the PR
body's 'the existing cloned builder and publish behavior stay unchanged'
denies. Commit() is unaffected because it ships meta.updates to the catalog
rather than built metadata.
##########
table/transaction.go:
##########
@@ -172,47 +172,61 @@ func (t *Transaction) apply(updates []Update, reqs
[]Requirement) error {
return errors.New("cannot apply updates to nil metadata")
}
- current, err := stagedMeta.Build()
- if err != nil {
- return err
- }
+ // Only new requirement validation needs the immutable metadata view.
+ // Updates can be applied directly to the staged builder, and duplicate
+ // requirements only need conflict checks.
+ var current Metadata
// Deduplicate requirements by semantic key, rejecting pairs that
// share a key but cannot both hold (see checkRequirementConflict).
// Only requirements actually appended are validated against the
// staged metadata: a deduplicated twin re-asserts base state the
// staged metadata has intentionally moved past, and its kept twin
// was validated when first added.
- existing := make(map[string]Requirement, len(t.reqs))
- stagedReqs := make([]Requirement, 0, len(t.reqs)+len(reqs))
- for _, r := range t.reqs {
- key, err := requirementSemanticKey(r)
- if err != nil {
- return err
- }
-
- existing[key] = r
- stagedReqs = append(stagedReqs, r)
- }
+ stagedReqs := t.reqs
+ if len(reqs) > 0 {
Review Comment:
**nit** — len(reqs)==0 now skips semantic-key computation over existing
t.reqs
Previously every apply re-ran requirementSemanticKey over t.reqs, so a
requirement that failed to marshal would error even on a no-requirement apply.
That early error is now skipped. Unreachable in practice since every
requirement in t.reqs had its key computed successfully when it was inserted,
so this is behavior-preserving; noting only because it is a silent removal of a
check.
##########
table/transaction_internal_test.go:
##########
@@ -1266,6 +1267,70 @@ func
TestTransactionApplyKeepsRequirementsUnchangedOnUpdateFailure(t *testing.T)
require.Equal(t, AssertTableUUID(baseMeta.TableUUID()), txn.reqs[0])
}
+func TestTransactionApplyDefersMetadataBuildForNoopRequirements(t *testing.T) {
+ t.Run("no requirements", func(t *testing.T) {
+ txn, _ := createTestTransactionWithMemIO(t,
*iceberg.UnpartitionedSpec)
+ txn.meta.defaultSpecID = 999
+
+ require.NoError(t, txn.apply(nil, nil))
+ _, err := txn.meta.Build()
+ require.ErrorIs(t, err, ErrInvalidMetadata)
+ })
+
+ t.Run("duplicate requirement", func(t *testing.T) {
+ txn, _ := createTestTransactionWithMemIO(t,
*iceberg.UnpartitionedSpec)
+ requirement := AssertCurrentSchemaID(0)
+ require.NoError(t, txn.apply(nil, []Requirement{requirement}))
+
+ // A duplicate requirement only needs its conflict check. Make
Build fail
+ // after the first apply so this path cannot accidentally
rebuild metadata.
+ txn.meta.defaultSpecID = 999
+ require.NoError(t, txn.apply(nil, []Requirement{requirement}))
+ _, err := txn.meta.Build()
+ require.ErrorIs(t, err, ErrInvalidMetadata)
+ })
+}
+
+func TestTransactionStagedTableBuildsOnePreviousMetadataLogEntry(t *testing.T)
{
+ txn, _ := createTestTransactionWithMemIO(t, *iceberg.UnpartitionedSpec)
Review Comment:
**minor** — TestTransactionStagedTableBuildsOnePreviousMetadataLogEntry
overstates the invariant it pins
The test name asserts a general 'one previous metadata log entry' property,
but the underlying non-idempotency of MetadataBuilder.Build()
(AppendMetadataLog has no dedupe guard) is untouched. The duplication is only
avoided on the no-new-requirement path. A transaction that adds a distinct
requirement per operation, or that calls StagedTable() more than once, still
accumulates duplicates. Consider renaming to reflect the no-requirement path,
or fixing AppendMetadataLog to be idempotent.
--
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]
