laskoviymishka commented on code in PR #1286:
URL: https://github.com/apache/iceberg-go/pull/1286#discussion_r3466962209
##########
table/orphan_cleanup.go:
##########
@@ -424,64 +422,7 @@ func walkDirectory(fsys iceio.IO, root string, fn
func(path string, info stdfs.F
})
}
- // Fallback to original implementation for IO types that don't
- // implement ListableIO yet.
- switch v := fsys.(type) {
- case iceio.LocalFS:
- cleanRoot := strings.TrimPrefix(root, "file://")
- if cleanRoot == "" {
- cleanRoot = "."
- }
-
- return filepath.WalkDir(cleanRoot, makeFileWalkFunc(fn,
func(path string) string {
- return path
- }))
-
- default:
- // For blob storage: direct field access since we know the
structure.
- bucket := getBucketName(v)
-
- parsed, err := url.Parse(root)
- if err != nil {
- return fmt.Errorf("invalid URL %s: %w", root, err)
- }
-
- walkPath := strings.TrimPrefix(parsed.Path, "/")
- if walkPath == "" {
- walkPath = "."
- }
-
- return stdfs.WalkDir(bucket, walkPath, makeFileWalkFunc(fn,
func(path string) string {
- return parsed.Scheme + "://" + parsed.Host + "/" + path
- }))
- }
-}
-
-// getBucketName gets the Bucket field from blob storage - absolute minimal
approach.
-func getBucketName(fsys iceio.IO) stdfs.FS {
- v := reflect.ValueOf(fsys).Elem()
-
- return v.FieldByName("Bucket").Interface().(stdfs.FS)
-}
-
-// makeFileWalkFunc creates a WalkDirFunc that processes only files with path
transformation.
-func makeFileWalkFunc(fn func(path string, info stdfs.FileInfo) error,
pathTransform func(string) string) stdfs.WalkDirFunc {
- return func(path string, d stdfs.DirEntry, err error) error {
- if err != nil {
- return err
- }
-
- if d.IsDir() {
- return nil
- }
-
- info, err := d.Info()
- if err != nil {
- return err
- }
-
- return fn(pathTransform(path), info)
- }
+ return fmt.Errorf("filesystem %T does not implement io.ListableIO",
fsys)
Review Comment:
The type here is `iceio.ListableIO`, but `io` is the stdlib alias also in
scope, so this error points readers at the wrong package — they'll go searching
stdlib `io` and come up empty.
I'd name the local alias: `fmt.Errorf("filesystem %T does not implement
iceio.ListableIO", fsys)`. The test assertion at `orphan_cleanup_test.go:632`
hard-codes the current string, so it needs to move with this.
##########
table/orphan_cleanup.go:
##########
@@ -404,7 +403,6 @@ func (t Table) getReferencedFiles(ctx context.Context, fs
iceio.IO, maxConcurren
}
func walkDirectory(fsys iceio.IO, root string, fn func(path string, info
stdfs.FileInfo) error) error {
- // Prefer ListableIO when available.
if listable, ok := fsys.(iceio.ListableIO); ok {
Review Comment:
`blobFileIO` (S3/GCS/ADLS) is now the load-bearing ListableIO type for the
whole gocloud path, but it has no compile-time assertion the way `LocalFS` does
— if its `WalkDir` signature drifts, the break surfaces at runtime in orphan
cleanup rather than at compile time.
I'd add `var _ icebergio.ListableIO = (*blobFileIO)(nil)` over in
`io/gocloud/blob.go` to lock it down. wdyt?
##########
table/orphan_cleanup.go:
##########
@@ -424,64 +422,7 @@ func walkDirectory(fsys iceio.IO, root string, fn
func(path string, info stdfs.F
})
}
- // Fallback to original implementation for IO types that don't
- // implement ListableIO yet.
- switch v := fsys.(type) {
- case iceio.LocalFS:
- cleanRoot := strings.TrimPrefix(root, "file://")
- if cleanRoot == "" {
- cleanRoot = "."
- }
-
- return filepath.WalkDir(cleanRoot, makeFileWalkFunc(fn,
func(path string) string {
- return path
- }))
-
- default:
- // For blob storage: direct field access since we know the
structure.
- bucket := getBucketName(v)
-
- parsed, err := url.Parse(root)
- if err != nil {
- return fmt.Errorf("invalid URL %s: %w", root, err)
- }
-
- walkPath := strings.TrimPrefix(parsed.Path, "/")
- if walkPath == "" {
- walkPath = "."
- }
-
- return stdfs.WalkDir(bucket, walkPath, makeFileWalkFunc(fn,
func(path string) string {
- return parsed.Scheme + "://" + parsed.Host + "/" + path
- }))
- }
-}
-
-// getBucketName gets the Bucket field from blob storage - absolute minimal
approach.
-func getBucketName(fsys iceio.IO) stdfs.FS {
- v := reflect.ValueOf(fsys).Elem()
-
- return v.FieldByName("Bucket").Interface().(stdfs.FS)
-}
-
-// makeFileWalkFunc creates a WalkDirFunc that processes only files with path
transformation.
-func makeFileWalkFunc(fn func(path string, info stdfs.FileInfo) error,
pathTransform func(string) string) stdfs.WalkDirFunc {
- return func(path string, d stdfs.DirEntry, err error) error {
- if err != nil {
- return err
- }
-
- if d.IsDir() {
- return nil
- }
-
- info, err := d.Info()
- if err != nil {
- return err
- }
-
- return fn(pathTransform(path), info)
- }
+ return fmt.Errorf("filesystem %T does not implement io.ListableIO",
fsys)
Review Comment:
`DeleteOrphanFiles` and `FSysF` are public, and this turns "IO that
implements only `iceio.IO`" into a hard runtime failure on orphan cleanup. For
all the built-in schemes that's fine — they all implement ListableIO — but a
downstream caller injecting a custom IO would hit this with no signal until
runtime.
I'd add a line of godoc on `DeleteOrphanFiles` noting that the table's IO
must implement `iceio.ListableIO`. Worth aligning with `PurgeFiles` too while
we're here — it does its own inline ListableIO check and silently skips the
walk, so the two paths handle the same condition in opposite ways. Not
blocking, but the asymmetry is a future trap.
##########
table/orphan_cleanup_test.go:
##########
@@ -621,6 +622,16 @@ func TestDeleteFilesFallsBackToExistingBehavior(t
*testing.T) {
assert.ElementsMatch(t, orphans, deleted)
}
+func TestWalkDirectoryRequiresListableIO(t *testing.T) {
+ err := walkDirectory(&mockPlainIO{}, "s3://bucket/data", func(string,
stdfs.FileInfo) error {
+ require.Fail(t, "walk callback should not run for non-listable
IO")
+
+ return nil
+ })
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "does not implement io.ListableIO")
Review Comment:
This is the only unit test touching `walkDirectory`, and it only covers the
error branch. With the fallback gone, nothing at the unit level exercises the
success path — if the ListableIO callback regresses (path transform,
file-vs-dir filtering, callback ordering) the integration tests would catch it
but they're build-tag gated and don't run in normal CI.
Could we add a case that drives a `MemFS` (or a small ListableIO mock) with
a few seeded paths through `walkDirectory` and asserts the callback receives
the full expected set? Otherwise the test would still pass if `walkDirectory`'s
body were deleted down to the error return. wdyt?
--
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]
