laskoviymishka commented on code in PR #1388:
URL: https://github.com/apache/iceberg-go/pull/1388#discussion_r3557628266
##########
table/update_spec.go:
##########
@@ -54,6 +55,26 @@ 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 {
Review Comment:
the guard here only fires when `t` is non-nil — if `t` (or `t.tbl`) is nil,
`us.err` stays nil, we skip the `return us`, and fall straight through to
`t.tbl.Metadata()` on line 80 and panic. `NewUpdateSchema` has the same shape a
few lines down at `txn.meta.LastColumnID()`. it's the exact bug this PR is
closing everywhere else.
to be fair, a nil `*Transaction` isn't constructable through the public API
— these are always called off a live transaction — so this is defense-in-depth,
not a live crash. but since we're already paying for the guard I'd set an
explicit error in the nil branch instead of skipping it:
```go
if t == nil {
us.err = errors.New("transaction is nil")
return us
}
us.err = t.ensureInitialized()
```
wdyt?
##########
table/transaction.go:
##########
@@ -93,10 +94,25 @@ type Transaction struct {
committed bool
}
+func (t *Transaction) ensureInitialized() error {
Review Comment:
not for this PR, but flagging it while we're here: this leaves ~20 entry
points each responsible for remembering to call `ensureInitialized()`, with
nothing enforcing it — the round-1 miss on `UpdateSchema`/`UpdateSpec` was
exactly this failure mode.
longer term I'd rather funnel meta access through a single accessor
(something like `txnMeta() (*MetadataBuilder, error)`) so a new method
physically can't forget the guard. non-blocking and probably its own follow-up
— just don't want us to lose the thread.
##########
table/table.go:
##########
@@ -131,22 +131,55 @@ func (t Table) LocationProvider() (LocationProvider,
error) {
}
func (t Table) NewTransaction() *Transaction {
- return t.NewTransactionOnBranch(MainBranch)
+ txn, err := t.NewTransactionOnBranchE(MainBranch)
+ if err != nil {
+ return t.newBrokenTransaction(MainBranch, err)
+ }
+
+ return txn
}
// NewTransactionOnBranch creates a new transaction that commits to the named
// branch. Use [NewTransaction] to commit to the default "main" branch.
func (t Table) NewTransactionOnBranch(branch string) *Transaction {
- meta, _ := MetadataBuilderFromBase(t.metadata, t.metadataLocation)
+ txn, err := t.NewTransactionOnBranchE(branch)
+ if err != nil {
+ return t.newBrokenTransaction(branch, err)
+ }
+
+ return txn
+}
+func (t Table) newBrokenTransaction(branch string, err error) *Transaction {
return &Transaction{
- tbl: &t,
- meta: meta,
- branch: branch,
- reqs: []Requirement{},
+ tbl: &t,
+ initErr: err,
+ branch: branch,
+ reqs: []Requirement{},
}
Review Comment:
not a blocker for me, but this one still feels off: there's no other
`...E`-suffixed constructor anywhere in the codebase, and the Go idiom would
usually make the error-returning form the primary one (or add a `Must`-style
wrapper going the other way) rather than an `E` variant. the closest analogues
we already have are the swallow-error `NewTransaction` /
`NewTransactionOnBranch` right above — so whatever we pick here sets the
precedent for how that pair evolves too.
since it's public surface we can't rename later without a break, I'd rather
not decide it unilaterally. @zeroshade — do you have a preference on the naming
convention here? happy to go with whatever you'd bless.
--
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]
