Copilot commented on code in PR #2342:
URL: https://github.com/apache/iceberg-go/pull/2342#discussion_r4405449438
##########
table/metadata.go:
##########
@@ -953,10 +953,11 @@ func (b *MetadataBuilder) addSnapshotInternal(snapshot
*Snapshot, preserveUpdate
} else if b.formatVersion >= 2 &&
snapshot.ParentSnapshotID != nil &&
snapshot.SequenceNumber <= lastSeqNum {
- return fmt.Errorf("can't add snapshot with sequence number %d,
must be > than last sequence number %d",
- snapshot.SequenceNumber, lastSeqNum)
+ return fmt.Errorf("%w: can't add snapshot with sequence number
%d, must be > than last sequence number %d",
Review Comment:
The phrase \"must be > than\" is grammatically awkward and harder to read.
Consider changing it to \"must be greater than\" (or \"must be >\") and
updating the affected tests that assert on this message.
##########
table/metadata_builder_internal_test.go:
##########
@@ -1928,6 +1928,57 @@ func TestAddSnapshotOnBuilderWithoutBaseV3(t *testing.T)
{
require.Equal(t, addedRows, meta.NextRowID())
}
+func TestAddSnapshotStaleValuesAreCommitConflicts(t *testing.T) {
+ now := time.Now().UnixMilli()
+ parentID := int64(1)
+
+ t.Run("sequence number taken by a peer", func(t *testing.T) {
+ builder := freshMetadataBuilder(t, 2)
+ peer := freshBuilderSnapshot(1, nil, 1, now)
+ require.NoError(t, builder.AddSnapshot(&peer))
+ stale := freshBuilderSnapshot(2, &parentID, 1, now)
+
+ err := builder.AddSnapshot(&stale)
+ require.ErrorIs(t, err, ErrCommitFailed)
+ require.ErrorContains(t, err, "can't add snapshot with sequence
number 1, must be > than last sequence number 1")
Review Comment:
This assertion is quite brittle because it hard-codes the full formatted
message including specific numeric values. To reduce future churn (e.g., if
message wording changes), consider asserting on a smaller stable substring (or
multiple substrings) while keeping the ErrorIs checks for ErrCommitFailed as
the primary behavioral assertion.
##########
table/metadata.go:
##########
@@ -1012,8 +1013,8 @@ func (b *MetadataBuilder)
validateAndUpdateRowLineage(snapshot *Snapshot) error
nextRowID := b.currentNextRowID()
if *snapshot.FirstRowID < nextRowID {
- return fmt.Errorf("%w: first-row-id %d is behind table
next-row-id %d",
- ErrInvalidRowLineage, *snapshot.FirstRowID, nextRowID)
+ return fmt.Errorf("%w: %w: first-row-id %d is behind table
next-row-id %d",
+ ErrCommitFailed, ErrInvalidRowLineage,
*snapshot.FirstRowID, nextRowID)
Review Comment:
Using multiple %w in fmt.Errorf reduces compatibility and can fail to
compile on Go toolchains that don’t support multi-error wrapping via
fmt.Errorf. Prefer wrapping via errors.Join (or a single wrapped error that
joins ErrCommitFailed and ErrInvalidRowLineage) so ErrorIs works for both while
keeping broader Go version compatibility.
--
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]
