zeroshade commented on code in PR #1657:
URL: https://github.com/apache/iceberg-go/pull/1657#discussion_r3732113884
##########
table/table.go:
##########
@@ -183,6 +183,12 @@ func (t Table) newBrokenTransaction(branch string, err
error) *Transaction {
// callers to receive the precise initialization error instead of hitting
// panic/undefined behavior later.
func (t Table) NewTransactionOnBranchWithError(branch string) (*Transaction,
error) {
+ for name, ref := range t.metadata.Refs() {
+ if name == branch && ref.SnapshotRefType != BranchRef {
Review Comment:
This type check runs only when the transaction is created. If the ref
changes from a branch to a tag at the same snapshot ID before commit,
`AssertRefSnapshotID` still succeeds because it checks only the ID, and the
set-ref update replaces the tag with a branch. #1659's UUID check does not help
because the table UUID is unchanged.
Suggested fix: validate that the target is still branch-typed on the atomic
commit/update path and after retry refreshes, then add a same-ID branch→tag
race test.
##########
table/table.go:
##########
@@ -183,6 +183,12 @@ func (t Table) newBrokenTransaction(branch string, err
error) *Transaction {
// callers to receive the precise initialization error instead of hitting
// panic/undefined behavior later.
func (t Table) NewTransactionOnBranchWithError(branch string) (*Transaction,
error) {
+ for name, ref := range t.metadata.Refs() {
+ if name == branch && ref.SnapshotRefType != BranchRef {
+ return nil, fmt.Errorf("%w: %s is not a branch",
iceberg.ErrInvalidArgument, branch)
Review Comment:
The error identifies the name but does not quote it or explain that tags
cannot be transaction targets.
Suggested fix: quote the ref name and state explicitly that a tag cannot be
used as a transaction target while continuing to wrap
`iceberg.ErrInvalidArgument`.
##########
table/table.go:
##########
@@ -183,6 +183,12 @@ func (t Table) newBrokenTransaction(branch string, err
error) *Transaction {
// callers to receive the precise initialization error instead of hitting
// panic/undefined behavior later.
func (t Table) NewTransactionOnBranchWithError(branch string) (*Transaction,
error) {
+ for name, ref := range t.metadata.Refs() {
Review Comment:
This loop continues scanning all refs even after it finds a matching branch.
Suggested fix: stop once the matching ref is found, or centralize this in a
typed ref lookup helper.
--
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]
