laskoviymishka commented on code in PR #1185:
URL: https://github.com/apache/iceberg-go/pull/1185#discussion_r3423886657
##########
catalog/hadoop/hadoop.go:
##########
@@ -526,29 +548,38 @@ func (c *Catalog) ListTables(_ context.Context, ns
table.Identifier) iter.Seq2[t
return
}
- entries, err := c.filesystem.ReadDir(nsPath)
- if err != nil {
- yield(nil, fmt.Errorf("hadoop catalog: failed to read
namespace directory: %w", err))
-
- return
- }
+ err = c.filesystem.WalkDir(nsPath, func(path string, d
fs.DirEntry, err error) error {
+ if err != nil {
+ return err
+ }
- for _, e := range entries {
- if !e.IsDir() {
- continue
+ // Anything that is a file is not a namespace or table,
so skip it.
+ if !d.IsDir() {
+ return nil
}
- child := filepath.Join(nsPath, e.Name())
- if !isTableDir(c.filesystem, child) {
- continue
+ // Skip the namespace directory itself.
+ if path == nsPath {
+ return nil
}
+ // Skip anything that is not a table directory.
+ if !isTableDir(c.filesystem, path) {
+ return fs.SkipDir
+ }
ident := make(table.Identifier, len(ns)+1)
copy(ident, ns)
- ident[len(ns)] = e.Name()
+ ident[len(ns)] = d.Name()
if !yield(ident, nil) {
- return
+ return fs.SkipAll
}
+
+ return nil
Review Comment:
The SkipDir/SkipAll fix here looks right — child namespaces aren't flattened
anymore and the early break no longer panics. To answer your question on
zeroshade's thread: yes, I'd add a regression test so we can close it
confidently.
`TestListTablesDoesNotYieldTablesInChildNamespace`: create `ns/tbl1` and
`ns/child_ns/tbl2`, then assert `ListTables(["ns"])` returns exactly
`[["ns","tbl1"]]`. That's the exact shape zeroshade reproduced, and it'd catch
any future regression to the SkipDir branch.
One thing still here: after yielding a table we `return nil`, so WalkDir
descends one level into each table's own `data/`/`metadata/` before SkipDir-ing
them. Harmless for correctness, just extra reads — `return fs.SkipDir` after
the yield avoids it. wdyt?
##########
catalog/hadoop/hadoop.go:
##########
@@ -619,7 +649,20 @@ func (c *Catalog) DropNamespace(_ context.Context, ns
table.Identifier) error {
path := c.namespaceToPath(ns)
- entries, err := c.filesystem.ReadDir(path)
+ foundEntries := false
+ err := c.filesystem.WalkDir(path, func(p string, d fs.DirEntry, err
error) error {
Review Comment:
I think the ReadDir→WalkDir swap dropped a guard here. If a plain file sits
at `warehouse/<name>`, WalkDir calls the callback once with `p == path`, we
return nil, `foundEntries` stays false, and we fall through to
`c.filesystem.Remove(path)` — so we silently delete the file and return
success. The old `ReadDir(path)` returned ENOTDIR on a non-dir and the delete
never happened.
I'd Stat the path up front and bail with `ErrNoSuchNamespace` if it isn't a
directory, before the walk.
A test that pins it: write a plain file at `warehouse/notadir`, call
`DropNamespace(["notadir"])`, assert it errors and the file is still on disk.
--
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]
