laskoviymishka commented on code in PR #1388:
URL: https://github.com/apache/iceberg-go/pull/1388#discussion_r3559016949
##########
table/update_spec.go:
##########
@@ -54,6 +55,29 @@ type transformKey struct {
}
func NewUpdateSpec(t *Transaction, caseSensitive bool) *UpdateSpec {
+ us := &UpdateSpec{
+ txn: t,
+ nameToField: make(map[string]iceberg.PartitionField),
+ nameToAddedField: make(map[string]iceberg.PartitionField),
+ transformToField:
make(map[transformKey]iceberg.PartitionField),
+ transformToAddedField:
make(map[transformKey]iceberg.PartitionField),
+ renames: make(map[string]string),
+ addedTimeFields: make(map[int]iceberg.PartitionField),
+ caseSensitive: caseSensitive,
+ adds: make([]iceberg.PartitionField, 0),
+ deletes: make(map[int]bool),
+ }
+
+ if t == nil {
+ us.err = errors.New("transaction is nil")
Review Comment:
The one thing I'd tidy from the round-2 fix: every other init error wraps
`ErrInvalidMetadata` (via `ensureInitialized` and `MetadataBuilderFromBase`),
but the nil-txn case here — and the mirror in `NewUpdateSchema` — uses a bare
`errors.New`, so `errors.Is(err, ErrInvalidMetadata)` comes back false only for
this path.
I'd wrap it: `fmt.Errorf("%w: transaction is nil", ErrInvalidMetadata)`. If
you do, the two nil-txn tests should switch from `assert.EqualError` to
`require.ErrorIs(err, ErrInvalidMetadata)` so they aren't pinned to the exact
string. Non-blocking, but it closes the last inconsistency in the guard set.
##########
table/transaction.go:
##########
@@ -2156,6 +2224,10 @@ func (t *Transaction) Commit(ctx context.Context)
(*Table, error) {
t.mx.Lock()
defer t.mx.Unlock()
+ if err := t.ensureInitialized(); err != nil {
Review Comment:
The suite covers `TableCommit()` but not `Commit(ctx)`, which is the
terminal path most likely to be hit on a broken txn. I'd add one subtest
asserting `errors.Is(err, ErrInvalidMetadata)` on `txn.Commit(ctx)` alongside
the existing ones.
##########
table/transaction.go:
##########
@@ -94,10 +95,25 @@ type Transaction struct {
committed bool
}
+func (t *Transaction) ensureInitialized() error {
+ if t.initErr != nil {
Review Comment:
Defense-in-depth thought, not blocking: this reads `t.initErr` on the
receiver with no `t == nil` check, so it'd panic rather than error on a nil
`*Transaction`. Can't happen today — every constructor returns a non-nil txn,
so `rd.txn.ensureInitialized()` and friends are always called on a live
pointer. But since the whole point of the PR is turning panics into errors, a
one-line `if t == nil { return fmt.Errorf("%w: transaction is nil",
ErrInvalidMetadata) }` at the top would make the guard robust if a future
caller ever hands it a nil. wdyt?
--
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]
