laskoviymishka commented on code in PR #1452:
URL: https://github.com/apache/iceberg-go/pull/1452#discussion_r3580563523
##########
cmd/iceberg/clean_orphan_files.go:
##########
@@ -40,19 +40,22 @@ func runCleanOrphanFiles(ctx context.Context, output
Output, cat catalog.Catalog
opts := []table.OrphanCleanupOption{
table.WithFilesOlderThan(olderThan),
- table.WithDryRun(true),
}
if cmd.Location != "" {
opts = append(opts, table.WithLocation(cmd.Location))
}
- result, err := tbl.DeleteOrphanFiles(ctx, opts...)
+ plan, err := tbl.PlanOrphanFiles(ctx, opts...)
if err != nil {
output.Error(fmt.Errorf("orphan file scan failed: %w", err))
os.Exit(1)
}
+ result := table.OrphanCleanupResult{
Review Comment:
This hand-built result never populates `OrphanFiles`, and that quietly
breaks both the preview and the final output.
`buildCleanOrphanFilesResult` with `dryRun=false` intersects
`result.OrphanFiles` against `DeletedFiles` — but `OrphanFiles` is nil here, so
the intersection is empty. The text preview prints "No orphan files found."
right before we prompt to delete real files, and the final output reuses this
same `cliResult` (`deleteResult` from `ExecuteOrphanCleanup` is discarded), so
the tool reports `OrphanFileCount: 0` even after it deletes the full set.
The plan already carries the per-file entries privately. I'd add a public
accessor — `func (p OrphanCleanupPlan) OrphanFiles() []OrphanFile { return
slices.Clone(p.orphanFiles) }` — populate `result.OrphanFiles` from it for the
preview, and rebuild `cliResult` from `deleteResult` after
`ExecuteOrphanCleanup` before the final output. wdyt?
##########
cmd/iceberg/clean_orphan_files.go:
##########
@@ -61,29 +64,26 @@ func runCleanOrphanFiles(ctx context.Context, output
Output, cat catalog.Catalog
return
}
- if len(result.OrphanFileLocations) == 0 {
+ if len(plan.Files()) == 0 {
Review Comment:
`plan.Files()` clones the slice on every call, and we hit it three times in
this function (this guard, the prompt, the result build). Minor, but I'd pull
it into a local once — or add a `Len() int` on the plan for the count-only
checks.
##########
table/orphan_cleanup.go:
##########
@@ -175,12 +175,40 @@ type OrphanFile struct {
SizeBytes int64
}
-// DeleteOrphanFiles identifies files under a table location that are no longer
-// referenced by table metadata and deletes them unless dry-run is enabled.
-//
-// The table filesystem must implement iceio.ListableIO so orphan cleanup can
-// fully enumerate candidate files before deciding what is safe to delete.
-func (t Table) DeleteOrphanFiles(ctx context.Context, opts
...OrphanCleanupOption) (OrphanCleanupResult, error) {
+// OrphanCleanupPlan contains the exact orphan files identified during one
scan.
+// The file list is private so callers can only obtain copies, keeping the plan
+// stable between confirmation and execution.
+type OrphanCleanupPlan struct {
+ orphanFileLocations []string
+ orphanFiles []OrphanFile
+ totalSizeBytes int64
+ cutoff time.Time
+}
+
+// Files returns a copy of the files in the cleanup plan.
+func (p OrphanCleanupPlan) Files() []string {
+ return slices.Clone(p.orphanFileLocations)
+}
+
+// TotalSizeBytes returns the combined size of files in the cleanup plan.
+func (p OrphanCleanupPlan) TotalSizeBytes() int64 {
+ return p.totalSizeBytes
+}
+
+// Cutoff returns the age cutoff used to create the cleanup plan.
+func (p OrphanCleanupPlan) Cutoff() time.Time {
Review Comment:
`Cutoff()` is only read by the test — no execution path uses it, and
`ExecuteOrphanCleanup` doesn't re-check age against it. As a public accessor it
reads like an execute-time guard that doesn't exist. I'd either drop it, or
godoc it as informational: the plan-time filter, not enforced at execution.
##########
cmd/iceberg/clean_orphan_files.go:
##########
@@ -93,6 +93,15 @@ func runCleanOrphanFiles(ctx context.Context, output Output,
cat catalog.Catalog
output.CleanOrphanFilesResult(cliResult)
}
+func shouldPrintCleanOrphanPreview(output Output) bool {
+ switch output.(type) {
+ case jsonOutput, *jsonOutput:
Review Comment:
`jsonOutput` is only ever constructed as a value, so the `*jsonOutput` arm
is dead outside the test — I'd drop it. Separately, `isMachineReadable` reads a
touch clearer than `shouldPrintCleanOrphanPreview`, since that's really the
property we're keying on.
##########
table/orphan_cleanup_test.go:
##########
@@ -86,6 +86,45 @@ func TestOrphanCleanupOptions(t *testing.T) {
assert.Equal(t, authorities, cfg.equalAuthorities)
}
+func TestOrphanCleanupPlanDoesNotExpandAfterPlanning(t *testing.T) {
+ ctx := context.Background()
+ fs := io.NewMemFS()
+ location := "mem://plan-race/table"
+ metadataLocation := location + "/metadata/v1.metadata.json"
+
+ meta, err := NewMetadata(iceberg.NewSchema(0), nil, UnsortedSortOrder,
location, nil)
+ require.NoError(t, err)
+ require.NoError(t, fs.WriteFile(metadataLocation, nil))
+
+ plannedOrphan := location + "/data/planned.parquet"
+ newOrphan := location + "/data/appeared-after-confirmation.parquet"
+ require.NoError(t, fs.WriteFile(plannedOrphan, []byte("planned")))
+
+ tbl := New(
+ []string{"db", "plan_race"},
+ meta,
+ metadataLocation,
+ func(context.Context) (io.IO, error) { return fs, nil },
+ nil,
+ )
+
+ plan, err := tbl.PlanOrphanFiles(ctx, WithFilesOlderThan(time.Hour))
Review Comment:
The don't-expand-after-planning behavior this asserts is good and worth
having. But `WithFilesOlderThan(time.Hour)` isn't exercising the age filter —
MemFS reports a zero `ModTime`, so every file is "older than" any cutoff and
the option is a no-op here. If we want real coverage of the age boundary, a
separate case with actual mtimes that asserts a recent file gets excluded would
do it.
I'd also assert `result.OrphanFiles` here, not just `DeletedFiles` — the
path/size pairs are exactly what broke in the CLI.
##########
table/orphan_cleanup.go:
##########
@@ -192,23 +220,58 @@ func (t Table) DeleteOrphanFiles(ctx context.Context,
opts ...OrphanCleanupOptio
equalAuthorities: nil, // no authority
equivalence by default
}
- // Apply functional options
for _, opt := range opts {
opt(cfg)
}
- return t.executeOrphanCleanup(ctx, cfg)
+ return cfg
+}
+
+// DeleteOrphanFiles identifies files under a table location that are no longer
+// referenced by table metadata and deletes them unless dry-run is enabled.
+//
+// The table filesystem must implement iceio.ListableIO so orphan cleanup can
+// fully enumerate candidate files before deciding what is safe to delete.
+func (t Table) DeleteOrphanFiles(ctx context.Context, opts
...OrphanCleanupOption) (OrphanCleanupResult, error) {
+ cfg := newOrphanCleanupConfig(opts...)
+ plan, err := t.planOrphanFiles(ctx, cfg)
+ if err != nil {
+ return OrphanCleanupResult{}, err
+ }
+ if cfg.dryRun {
+ return plan.result(), nil
+ }
+
+ return t.executeOrphanCleanup(ctx, plan, cfg)
+}
+
+// PlanOrphanFiles identifies orphan files without deleting anything. The
+// returned plan can be shown to a user and passed to ExecuteOrphanCleanup to
+// delete exactly that set.
+func (t Table) PlanOrphanFiles(ctx context.Context, opts
...OrphanCleanupOption) (OrphanCleanupPlan, error) {
+ return t.planOrphanFiles(ctx, newOrphanCleanupConfig(opts...))
+}
+
+// ExecuteOrphanCleanup deletes exactly the files in plan. It does not perform
+// another orphan scan, so files appearing after planning are not included.
+func (t Table) ExecuteOrphanCleanup(ctx context.Context, plan
OrphanCleanupPlan, opts ...OrphanCleanupOption) (OrphanCleanupResult, error) {
Review Comment:
`executeOrphanCleanup` only reads `deleteFunc` and `maxConcurrency` off
`cfg`, but `ExecuteOrphanCleanup` accepts the full `OrphanCleanupOption` set —
so `WithFilesOlderThan`, `WithLocation`, `WithPrefixMismatchMode` etc. are
silently dropped here. Someone calling `ExecuteOrphanCleanup(ctx, plan,
WithFilesOlderThan(24*time.Hour))` gets no error and no effect, which is a
nasty footgun given the plan's whole point is that its set is already fixed.
I'd narrow this to a smaller option type (just
`WithDeleteFunc`/`WithMaxConcurrency`/`WithDryRun`), so the scan-time options
aren't even expressible on the execute path. wdyt?
##########
cmd/iceberg/clean_orphan_files_test.go:
##########
@@ -171,6 +171,12 @@ func
TestBuildCleanOrphanFilesResultDeletedFiltersToDeletedFiles(t *testing.T) {
assert.Equal(t, int64(2048), result.OrphanFiles[0].SizeBytes)
}
+func TestShouldPrintCleanOrphanPreview(t *testing.T) {
Review Comment:
This covers the predicate, but the behavior the PR is actually selling — the
text preview listing the exact files before we prompt — has no test. An
end-to-end `runCleanOrphanFiles` case that captures text output and asserts the
preview contains the orphan paths would have caught the empty-preview bug I
flagged in `clean_orphan_files.go`; right now nothing does. Could we add one?
--
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]
