tanmayrauth commented on code in PR #1412:
URL: https://github.com/apache/iceberg-go/pull/1412#discussion_r3525957871
##########
table/orphan_cleanup.go:
##########
@@ -161,11 +161,17 @@ func WithEqualAuthorities(authorities map[string]string)
OrphanCleanupOption {
type OrphanCleanupResult struct {
OrphanFileLocations []string
+ OrphanFiles []OrphanFile
Review Comment:
`OrphanFiles []OrphanFile` now carries the path *and* size for every orphan,
while `OrphanFileLocations []string` above it carries just the paths — so the
paths are duplicated across the two fields. Keeping `OrphanFileLocations` for
backward compat is the right call, but could you add a one-line doc comment
noting it's retained for compatibility and that `OrphanFiles` is the richer
form? Otherwise a future reader has to diff the two to figure out why both
exist.
##########
cmd/iceberg/clean_orphan_files.go:
##########
@@ -99,9 +99,17 @@ func buildCleanOrphanFilesResult(tbl *table.Table, result
table.OrphanCleanupRes
files = result.DeletedFiles
}
+ orphanSizes := make(map[string]int64, len(result.OrphanFiles))
+ for _, f := range result.OrphanFiles {
+ orphanSizes[f.Path] = f.SizeBytes
+ }
+
entries := make([]OrphanFileEntry, 0, len(files))
for _, f := range files {
- entries = append(entries, OrphanFileEntry{Path: f})
+ entries = append(entries, OrphanFileEntry{
+ Path: f,
+ SizeBytes: orphanSizes[f],
Review Comment:
`SizeBytes: orphanSizes[f]` — in the non-dry-run branch `files` comes from
`result.DeletedFiles`, but this size map is built from `result.OrphanFiles`.
Any deleted path not present in `OrphanFiles` would silently render as `0 B`
rather than being caught. Today `DeletedFiles ⊆ OrphanFiles` so it's safe, but
that's an implicit invariant — worth a short comment, or build the deleted
entries by filtering `OrphanFiles` against the deleted set so the size always
stays attached to its path.
##########
cmd/iceberg/maintenance.go:
##########
@@ -172,7 +172,7 @@ type CleanOrphanFilesResult struct {
type OrphanFileEntry struct {
Path string `json:"path"`
- SizeBytes int64 `json:"size_bytes,omitempty"`
+ SizeBytes int64 `json:"size_bytes"`
Review Comment:
Dropping `omitempty` means a genuinely 0-byte orphan now always serializes
`"size_bytes":0` instead of omitting the field. I think that's the right move
for a stable output schema (and it matches `PartitionStatsEntry.SizeBytes` at
line 146, which also has no omitempty) — just flagging it as an observable JSON
change for anyone parsing the output.
--
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]
