laskoviymishka commented on PR #1071: URL: https://github.com/apache/iceberg-go/pull/1071#issuecomment-4444555606
The `upgrade` path is clean — `tx.UpgradeFormatVersion` + `tx.Commit` is the right surface and the dry-run/confirmation flow matches the rest of the series. The concerns are concentrated in `runRollback`. It drops to `cat.CommitTable` directly with only `AssertTableUUID`, so a concurrent commit advancing `main` between the load and the commit is silently overwritten — both Java's `SetSnapshotOperation` and PyIceberg's `_set_ref_snapshot` always include `AssertRefSnapshotID(main, current)`, and `previousSnapshotID` is already computed here so threading it into the requirements is a small change. The same function also doesn't check that the target snapshot is an ancestor of current main, so today the command will happily re-point `main` to an unrelated snapshot from a different branch history — Java's `rollbackTo` and PyIceberg's `rollback_to_snapshot` both reject non-ancestors, and `table.IsAncestorOf` is already exported. Tests cover formatters only; nothing exercises `runRollback`/`runUpgrade` themselves, so the dry-run-no-commit and non-TTY-without-`--yes` lines from #957 are untested. A fake catalog whose `CommitTable` panics would cover that without Docker. Inline comments left on the specific lines. Once the rollback CAS + ancestry checks land this should be ready. -- 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]
