laskoviymishka commented on code in PR #1388:
URL: https://github.com/apache/iceberg-go/pull/1388#discussion_r3535328913
##########
table/transaction.go:
##########
@@ -206,10 +225,17 @@ func (t *Transaction) SetProperties(props
iceberg.Properties) error {
// UpgradeFormatVersion upgrades the table to the given format version.
Downgrading
// is not allowed. If the table is already at the given version, this is a
no-op.
func (t *Transaction) UpgradeFormatVersion(version int) error {
+ if err := t.ensureInitialized(); err != nil {
+ return err
+ }
+
return t.apply([]Update{NewUpgradeFormatVersionUpdate(version)}, nil)
}
func (t *Transaction) RollbackToSnapshot(snapshotID int64) error {
+ if err := t.ensureInitialized(); err != nil {
Review Comment:
`UpdateSchema` just below here (around line 252) never got this guard, and
it's the one method where the omission still panics instead of deferring —
`NewUpdateSchema` derefs `txn.meta.LastColumnID()` at construction before
anything checks `meta`. So on a broken txn, `txn.UpdateSchema()` panics, which
is precisely the case this PR is trying to eliminate.
Adding `ensureInitialized()` at the top of `Transaction.UpdateSchema` fixes
it (or pass the last-column-id by value the way `UpdateSpec` already does).
Either way I'd want a test that calls `UpdateSchema` on a broken txn.
##########
table/table.go:
##########
@@ -131,20 +131,43 @@ func (t Table) LocationProvider() (LocationProvider,
error) {
}
func (t Table) NewTransaction() *Transaction {
- return t.NewTransactionOnBranch(MainBranch)
+ txn, _ := t.NewTransactionOnBranchE(MainBranch)
+
+ 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, _ := t.NewTransactionOnBranchE(branch)
- return &Transaction{
- tbl: &t,
- meta: meta,
- branch: branch,
- reqs: []Requirement{},
+ return txn
+}
+
+// NewTransactionOnBranchE creates a new transaction and returns any metadata
+// initialization error that prevents builder construction.
+//
+// This preserves the old non-failing constructor contract while allowing
+// callers to receive the precise initialization error instead of hitting
+// panic/undefined behavior later.
+func (t Table) NewTransactionOnBranchE(branch string) (*Transaction, error) {
+ meta, err := MetadataBuilderFromBase(t.metadata, t.metadataLocation)
+ if err != nil {
+ return &Transaction{
Review Comment:
Returning a non-nil `*Transaction` alongside a non-nil error is a surprising
contract in Go — callers who check `err` first will drop the txn, and callers
who don't will keep a poisoned object.
I'd return `nil, err` here and let the legacy `NewTransaction` /
`NewTransactionOnBranch` wrappers build the poisoned-but-usable txn themselves.
That keeps the `(T, error)` signature idiomatic and confines the swallow to the
back-compat path.
##########
table/transaction.go:
##########
@@ -93,10 +94,25 @@ type Transaction struct {
committed bool
}
+func (t *Transaction) ensureInitialized() error {
+ if t.initErr != nil {
+ return t.initErr
+ }
+ if t.meta == nil {
+ return errors.New("transaction metadata builder is not
initialized")
+ }
+
+ return nil
+}
+
func (t *Transaction) apply(updates []Update, reqs []Requirement) error {
t.mx.Lock()
defer t.mx.Unlock()
+ if err := t.ensureInitialized(); err != nil {
Review Comment:
`apply()` funnels most mutations through this one guard, which is nice — but
every entry point that doesn't route through `apply` needs its own copy, and
it's easy to miss one. `UpdateSpec` surfaces the error late (at `Commit` rather
than at the call), and `TableCommit`, `WriteEqualityDeletes`, and
`RewriteFiles` aren't guarded at all.
Rather than hand-copying the guard into ~20 methods with no compile-time
enforcement, could we expose a `txnMeta() (*MetadataBuilder, error)` accessor
and have methods pull `meta` through it? Then a new method physically can't
touch `meta` without going through the check. wdyt?
##########
table/transaction_internal_test.go:
##########
@@ -60,6 +62,59 @@ func
TestTransactionApplyDedupesEquivalentRequirementsWithinAndAcrossCalls(t *te
requireContainsRefSnapshotRequirement(t, txn.reqs, MainBranch,
&mainSnapshotID)
}
+func TestNewTransactionOnBranchEReturnsTransactionInitError(t *testing.T) {
+ baseMeta, err := NewMetadata(simpleSchema(), iceberg.UnpartitionedSpec,
UnsortedSortOrder, "table-location", nil)
+ require.NoError(t, err, "new metadata")
+
+ txn, err := New(Identifier{"db", "broken"}, brokenMetadata{
+ Metadata: baseMeta,
+ }, "metadata.json", func(context.Context) (iceio.IO, error) {
+ return nil, nil
+ }, nil).NewTransactionOnBranchE(MainBranch)
+ require.Error(t, err, "expected metadata builder initialization to
fail")
+ require.ErrorContains(t, err, "current schema is missing")
+ require.ErrorIs(t, err, ErrInvalidMetadata)
+ require.NotNil(t, txn, "transaction should still be returned for
compatibility")
+}
+
+func TestNewTransactionOnBranchKeepsLegacySignatureAndFailsOnUse(t *testing.T)
{
+ baseMeta, err := NewMetadata(simpleSchema(), iceberg.UnpartitionedSpec,
UnsortedSortOrder, "table-location", nil)
+ require.NoError(t, err, "new metadata")
+
+ txn := New(Identifier{"db", "broken"}, brokenMetadata{
+ Metadata: baseMeta,
+ }, "metadata.json", func(context.Context) (iceio.IO, error) {
+ return nil, nil
+ }, nil).NewTransaction()
+ err = txn.SetProperties(iceberg.Properties{"k": "v"})
+ require.ErrorContains(t, err, "current schema is missing")
Review Comment:
This is the exact test that would've caught the `UpdateSchema` panic if it
exercised it — I'd add a `txn.UpdateSchema(...)` case here on the broken txn.
Two smaller things: this func drives both `SetProperties` and
`RowDelta.Commit`, so I'd split it into `t.Run` blocks; and `brokenMetadata`
overriding `CurrentSchema()` to nil is a bit synthetic. A round-trip through
`ParseMetadataBytes` with a missing current-schema-id would exercise the real
deserialization failure this PR cares about.
##########
table/table.go:
##########
@@ -131,20 +131,43 @@ func (t Table) LocationProvider() (LocationProvider,
error) {
}
func (t Table) NewTransaction() *Transaction {
- return t.NewTransactionOnBranch(MainBranch)
+ txn, _ := t.NewTransactionOnBranchE(MainBranch)
+
+ 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, _ := t.NewTransactionOnBranchE(branch)
- return &Transaction{
- tbl: &t,
- meta: meta,
- branch: branch,
- reqs: []Requirement{},
+ return txn
+}
+
+// NewTransactionOnBranchE creates a new transaction and returns any metadata
+// initialization error that prevents builder construction.
+//
+// This preserves the old non-failing constructor contract while allowing
+// callers to receive the precise initialization error instead of hitting
+// panic/undefined behavior later.
+func (t Table) NewTransactionOnBranchE(branch string) (*Transaction, error) {
Review Comment:
The `...E` suffix isn't a convention we use anywhere else in the codebase,
so the correct constructor ends up hidden behind an unfamiliar name while
`NewTransaction`/`NewTransactionOnBranch` quietly hand back a broken object.
I'd either spell it out (`NewTransactionOnBranchWithError`) or, better, make
the `(T, error)` form the canonical constructor and deprecate the swallowing
ones. Not blocking, but worth settling before this lands as public API.
##########
table/transaction.go:
##########
@@ -93,10 +94,25 @@ type Transaction struct {
committed bool
}
+func (t *Transaction) ensureInitialized() error {
+ if t.initErr != nil {
+ return t.initErr
+ }
+ if t.meta == nil {
Review Comment:
The `t.meta == nil` branch looks unreachable in practice — `initErr` is set
whenever `meta` is nil, so the first check always returns first.
If you're keeping it as a defensive net, I'd wrap it as `fmt.Errorf("%w:
...", ErrInvalidMetadata)` so it stays consistent with the `initErr` path — a
bare `errors.New` here wouldn't match `errors.Is(err, ErrInvalidMetadata)` if
the branch ever were reached.
--
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]
