laskoviymishka commented on code in PR #1566:
URL: https://github.com/apache/iceberg-go/pull/1566#discussion_r3666328654
##########
cmd/iceberg/main.go:
##########
@@ -564,23 +564,33 @@ func runDrop(ctx context.Context, output Output, cat
catalog.Catalog, cmd *DropC
output.Error(err)
osExit(1)
}
+
+ output.Text("Namespace " + cmd.Namespace.Identifier + " dropped
successfully")
case cmd.Table != nil:
ident := catalog.ToIdentifier(cmd.Table.Identifier)
- var err error
+
if cmd.Table.Purge {
- if purger, ok := cat.(catalog.PurgeableTable); ok {
- err = purger.PurgeTable(ctx, ident)
- } else {
+ purger, ok := cat.(catalog.PurgeableTable)
+ if !ok {
Review Comment:
The refactor to an early-return guard dropped the structural guarantee the
old if/else had here. `purger` is a nil interface when `!ok`, and the only
reason the `PurgeTable` call below doesn't nil-deref is that `osExit` never
returns.
That's a fragile invariant to lean on — the day someone swaps in an `osExit`
double that returns, this deadfalls. I'd add an explicit `return` right after
`osExit(1)`, matching what the success branch already does a few lines down.
wdyt?
##########
table/arrow_scanner.go:
##########
@@ -1024,7 +1024,7 @@ func (as *arrowScan) processRecords(
if err != nil {
return err
}
- out <- enumeratedRecord{Record:
internal.Enumerated[arrow.RecordBatch]{
+ out <- enumeratedRecord{Record:
tblutils.Enumerated[arrow.RecordBatch]{
Review Comment:
To close the loop on your PR note — this fix is correct. `tblutils` is
imported at line 38 and used throughout the file, so this compiles cleanly; it
was just a stale unaliased `internal.Enumerated` the rest of the file had
already migrated off.
The only thing I'd change is scope: it's unrelated to the CLI messages and
makes the PR non-atomic, which hurts bisection. I'd pull it into its own
one-line PR against the scan-planning work so this one stays focused. Not
blocking the fix itself.
##########
cmd/iceberg/main.go:
##########
@@ -564,23 +564,33 @@ func runDrop(ctx context.Context, output Output, cat
catalog.Catalog, cmd *DropC
output.Error(err)
osExit(1)
}
+
+ output.Text("Namespace " + cmd.Namespace.Identifier + " dropped
successfully")
case cmd.Table != nil:
ident := catalog.ToIdentifier(cmd.Table.Identifier)
- var err error
+
if cmd.Table.Purge {
- if purger, ok := cat.(catalog.PurgeableTable); ok {
- err = purger.PurgeTable(ctx, ident)
- } else {
+ purger, ok := cat.(catalog.PurgeableTable)
+ if !ok {
output.Error(fmt.Errorf("catalog %s does not
support purge", cat.CatalogType()))
osExit(1)
}
- } else {
- err = cat.DropTable(ctx, ident)
+
+ if err := purger.PurgeTable(ctx, ident); err != nil {
+ output.Error(err)
+ osExit(1)
+ }
+
+ output.Text("Table " + cmd.Table.Identifier + " purged
successfully")
+
+ return
}
- if err != nil {
+
+ if err := cat.DropTable(ctx, ident); err != nil {
output.Error(err)
osExit(1)
}
+ output.Text("Table " + cmd.Table.Identifier + " dropped
successfully")
Review Comment:
There's no blank line between the `DropTable` error block and this
`output.Text`, and nlreturn (on in `.golangci.yaml`) will flag it. Every other
site in this function has the blank line — the namespace and purge branches
both do — so this'll fail CI as-is. Just need a blank line in before line 593.
--
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]
