Revanth14 commented on PR #1170: URL: https://github.com/apache/iceberg-go/pull/1170#issuecomment-4637882451
> LGTM, the timestamp resolution reusing SnapshotAsOf on the snapshot log is the right call, and the off-lineage / array-order test cases nail down the semantics nicely. > > One small thing and one nit, neither blocking: > > * validateRollbackSelector runs both in main() before catalog init and again at the top of runRollback (upgrade_rollback.go:90). I think this is intentional so runRollback stays self-contained for the direct unit tests — just confirming that's the reasoning rather than a leftover? > * the --timestamp help text says "RFC3339" but parseRollbackTimestamp uses time.RFC3339Nano (upgrade_rollback.go:198), so it'll also accept fractional seconds. Strictly more lenient, so no harm, just flagging the wording in case you want them to match. > > Nice work, especially the subprocess test for validation-before-init. Thanks for reviewing @tanmayrauth. Yes, the double validation is intentional: - `main()` validates before catalog init so invalid selector usage fails with a clear CLI error instead of a catalog connection error. - `runRollback` keeps the command handler self-contained/testable and protects direct callers of the handler. I also updated the `--timestamp` help text to mention that fractional seconds are accepted. -- 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]
