C-Loftus commented on code in PR #1185:
URL: https://github.com/apache/iceberg-go/pull/1185#discussion_r3413693755
##########
catalog/hadoop/hadoop.go:
##########
@@ -526,29 +549,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 nil
+ }
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 nil
}
+
+ return nil
Review Comment:
Thank you very much for catching this. Could you please clarify what you
mean by `Both reproduced locally`? Did you run this code with a sample script?
At least from what I can see in both my tests and CI on github, this code did
not raise any errors in the test suite. Should another test be added to prevent
regressions on this?
--
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]
