zeroshade commented on code in PR #1436:
URL: https://github.com/apache/iceberg-go/pull/1436#discussion_r3566740806
##########
catalog/hadoop/hadoop.go:
##########
@@ -881,8 +881,10 @@ func (c *Catalog) PurgeTable(ctx context.Context,
identifier table.Identifier) e
log.Printf("WARNING: failing to purge some files in Hadoop
table %s: %v", identifier, purgeErr)
}
- // Delete the table directory root from the local storage
- return c.DropTable(ctx, identifier)
+ // PurgeFiles removes the metadata files that DropTable uses to
recognize a
+ // table. The table was already validated by LoadTable above, so remove
its
+ // directory directly instead of checking it again after the purge.
+ return c.filesystem.RemoveAll(c.tableToPath(identifier))
Review Comment:
This `RemoveAll` unconditionally removes the entire table directory (data
files included) after `PurgeFiles`. But `PurgeFiles` intentionally skips
data-file deletion when `gc.enabled=false` (see `table/orphan_cleanup.go`), so
this regresses that contract — the SQL catalog preserves data in that case (cf.
the `gc.enabled=false` test in `catalog/sql/sql_test.go`, and the
`PurgeableTable` docs in `catalog/catalog.go`).
Please gate the `RemoveAll` on the loaded table's `gc.enabled` property
(when GC is disabled, only remove metadata/empty dirs, not data), and add a
Hadoop `PurgeTable` test with `gc.enabled=false` that asserts referenced data
files survive.
--
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]
