You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/07/29 21:41:05 UTC
[PR] fix(io): make MemFS WalkDir emit directory entries [iceberg-go]
fallintoplace opened a new pull request, #1604:
URL: https://github.com/apache/iceberg-go/pull/1604
## Summary
- synthesize root and intermediate directory entries in `MemFS.WalkDir`
- emit entries in lexical order
- implement `SkipDir` and `SkipAll` behavior
- keep callbacks outside the filesystem lock
## Why
`ListableIO.WalkDir` requires callbacks for every file or directory, including the root. MemFS previously emitted only files in map iteration order, so directory-aware behavior and `SkipDir` differed from other filesystem implementations.
## Testing
- `go test ./io`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(io): make MemFS WalkDir emit directory entries [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1604:
URL: https://github.com/apache/iceberg-go/pull/1604
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(io): make MemFS WalkDir emit directory entries [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1604:
URL: https://github.com/apache/iceberg-go/pull/1604#discussion_r3681127518
##########
io/mem_test.go:
##########
@@ -257,5 +262,32 @@ func TestMemIO_WalkDirCallbackCanRemoveFiles(t *testing.T) {
return nil
}))
- assert.Empty(t, remaining)
+ assert.Equal(t, []string{"mem://bucket/root"}, remaining)
Review Comment:
This assertion now leans entirely on the synthetic root entry — `mem://bucket/root` was never written as a file, so it shows up in `remaining` only because WalkDir emits a directory node for it. The test still covers the deadlock fix, but the assertion is now checking synthetic-dir emission rather than file-removal-during-walk, and it'd pass just the same if someone added `mem://bucket/root` as a real file.
If the synthetic-root behavior changes per the ErrNotExist discussion, this expectation moves with it — so it's worth re-grounding the assertion on the real files once that lands.
##########
io/mem.go:
##########
@@ -107,28 +109,78 @@ func (m *MemFS) WriteFile(name string, content []byte) error {
func (m *MemFS) WalkDir(root string, fn fs.WalkDirFunc) error {
type walkEntry struct {
- path string
- size int64
+ path string
+ size int64
+ isDir bool
}
root = strings.TrimRight(root, "/")
m.mu.RLock()
- var entries []walkEntry
+ entriesByPath := make(map[string]walkEntry)
+ addEntry := func(entry walkEntry) {
+ current, exists := entriesByPath[entry.path]
+ if !exists || entry.isDir || !current.isDir {
Review Comment:
I think there's a real bug in this condition. `!exists || entry.isDir || !current.isDir` means a synthetic dir will happily overwrite a real file entry — the `entry.isDir` clause fires for any incoming directory regardless of what's already stored.
That bites when a key is both `root` and a prefix of other keys. Say `m.files` holds `mem://bucket/root` (a file) and `mem://bucket/root/a.txt`: the first key stores `{root, isDir:false}`, then walking the second calls `addEntry({root, isDir:true})`, which passes the `entry.isDir` clause and clobbers the file. Root gets reclassified as a directory and the real file entry is lost — `isTableDir`/`scanMetadataFiles` skip it via `d.IsDir()` and never see it.
I'd flip the precedence so real files always win over synthetic dirs — `!exists || (!entry.isDir && current.isDir)`. Same one-liner also fixes the broader-than-intent read that makes this condition hard to reason about. A regression test with root as a file key would lock it down.
##########
io/mem.go:
##########
@@ -107,28 +109,78 @@ func (m *MemFS) WriteFile(name string, content []byte) error {
func (m *MemFS) WalkDir(root string, fn fs.WalkDirFunc) error {
type walkEntry struct {
- path string
- size int64
+ path string
+ size int64
+ isDir bool
}
root = strings.TrimRight(root, "/")
m.mu.RLock()
- var entries []walkEntry
+ entriesByPath := make(map[string]walkEntry)
+ addEntry := func(entry walkEntry) {
+ current, exists := entriesByPath[entry.path]
+ if !exists || entry.isDir || !current.isDir {
+ entriesByPath[entry.path] = entry
+ }
+ }
for key, data := range m.files {
if !memPathInRoot(key, root) {
continue
}
- entries = append(entries, walkEntry{
- path: key,
- size: int64(len(data)),
- })
+
+ if key == root {
+ addEntry(walkEntry{path: key, size: int64(len(data))})
+
+ continue
+ }
+
+ addEntry(walkEntry{path: root, isDir: true})
+ relative := strings.TrimPrefix(key, root+"/")
+ parts := strings.Split(relative, "/")
+ for i := range parts {
+ entryPath := strings.Join(parts[:i+1], "/")
+ if root != "" {
+ entryPath = root + "/" + entryPath
+ }
+ entry := walkEntry{path: entryPath, isDir: i < len(parts)-1}
+ if !entry.isDir {
+ entry.size = int64(len(data))
+ }
+ addEntry(entry)
+ }
}
m.mu.RUnlock()
+ if len(entriesByPath) == 0 {
+ addEntry(walkEntry{path: root, isDir: true})
Review Comment:
This is where MemFS stops being a faithful stand-in for LocalFS. When nothing matches `root` — whether the dir is empty or simply doesn't exist — we synthesize a `{root, isDir:true}` entry and hand the callback a successful dir with a nil error.
`filepath.WalkDir` (what LocalFS delegates to) does the opposite: if the initial stat on root fails it calls `fn(root, nil, fs.ErrNotExist)`. So a test asserting the not-found path against MemFS silently passes where it'd correctly fail on a real FS. `hadoop.isTableDir` only comes out right here because its callback short-circuits on `path == metaDir`.
I'd have this branch return `fn(root, nil, &fs.PathError{Op: "lstat", Path: root, Err: fs.ErrNotExist})` and only synthesize the root dir when at least one key actually lives under it. If instead we want MemFS to not model directory existence at all, that's a defensible call — but then it's worth a doc comment saying so, because right now it reads like a bug. wdyt?
##########
io/mem.go:
##########
@@ -107,28 +109,78 @@ func (m *MemFS) WriteFile(name string, content []byte) error {
func (m *MemFS) WalkDir(root string, fn fs.WalkDirFunc) error {
type walkEntry struct {
- path string
- size int64
+ path string
+ size int64
+ isDir bool
}
root = strings.TrimRight(root, "/")
m.mu.RLock()
- var entries []walkEntry
+ entriesByPath := make(map[string]walkEntry)
+ addEntry := func(entry walkEntry) {
+ current, exists := entriesByPath[entry.path]
+ if !exists || entry.isDir || !current.isDir {
+ entriesByPath[entry.path] = entry
+ }
+ }
for key, data := range m.files {
if !memPathInRoot(key, root) {
continue
}
- entries = append(entries, walkEntry{
- path: key,
- size: int64(len(data)),
- })
+
+ if key == root {
+ addEntry(walkEntry{path: key, size: int64(len(data))})
+
+ continue
+ }
+
+ addEntry(walkEntry{path: root, isDir: true})
+ relative := strings.TrimPrefix(key, root+"/")
+ parts := strings.Split(relative, "/")
+ for i := range parts {
+ entryPath := strings.Join(parts[:i+1], "/")
+ if root != "" {
+ entryPath = root + "/" + entryPath
+ }
+ entry := walkEntry{path: entryPath, isDir: i < len(parts)-1}
+ if !entry.isDir {
+ entry.size = int64(len(data))
+ }
+ addEntry(entry)
+ }
}
m.mu.RUnlock()
+ if len(entriesByPath) == 0 {
+ addEntry(walkEntry{path: root, isDir: true})
+ }
- for _, entry := range entries {
- info := &memFileInfo{name: filepath.Base(entry.path), size: entry.size}
- if err := fn(entry.path, fs.FileInfoToDirEntry(info), nil); err != nil {
+ paths := make([]string, 0, len(entriesByPath))
+ for entryPath := range entriesByPath {
+ paths = append(paths, entryPath)
+ }
+ sort.Strings(paths)
+
+ var skipPrefix string
+ for _, entryPath := range paths {
+ if skipPrefix != "" && strings.HasPrefix(entryPath, skipPrefix) {
+ continue
+ }
+
+ entry := entriesByPath[entryPath]
+ info := &memFileInfo{name: path.Base(entry.path), size: entry.size, isDir: entry.isDir}
Review Comment:
WalkDir now uses `path.Base` here, but `Open` still uses `filepath.Base` (line 83, just outside this diff). On Windows those disagree for `mem://` keys — `filepath.Base` splits on backslash and would return the whole string, so `Stat().Name()` differs depending on whether you reached the file via Open or WalkDir.
Since these are URI keys, `path.Base` is the right one everywhere. I'd switch line 83 too, and `path/filepath` looks unused after that so it can probably go.
##########
io/mem.go:
##########
@@ -107,28 +109,78 @@ func (m *MemFS) WriteFile(name string, content []byte) error {
func (m *MemFS) WalkDir(root string, fn fs.WalkDirFunc) error {
type walkEntry struct {
- path string
- size int64
+ path string
+ size int64
+ isDir bool
}
root = strings.TrimRight(root, "/")
m.mu.RLock()
- var entries []walkEntry
+ entriesByPath := make(map[string]walkEntry)
+ addEntry := func(entry walkEntry) {
+ current, exists := entriesByPath[entry.path]
+ if !exists || entry.isDir || !current.isDir {
+ entriesByPath[entry.path] = entry
+ }
+ }
for key, data := range m.files {
if !memPathInRoot(key, root) {
continue
}
- entries = append(entries, walkEntry{
- path: key,
- size: int64(len(data)),
- })
+
+ if key == root {
+ addEntry(walkEntry{path: key, size: int64(len(data))})
+
+ continue
+ }
+
+ addEntry(walkEntry{path: root, isDir: true})
+ relative := strings.TrimPrefix(key, root+"/")
+ parts := strings.Split(relative, "/")
+ for i := range parts {
+ entryPath := strings.Join(parts[:i+1], "/")
+ if root != "" {
+ entryPath = root + "/" + entryPath
+ }
+ entry := walkEntry{path: entryPath, isDir: i < len(parts)-1}
+ if !entry.isDir {
+ entry.size = int64(len(data))
+ }
+ addEntry(entry)
+ }
}
m.mu.RUnlock()
+ if len(entriesByPath) == 0 {
+ addEntry(walkEntry{path: root, isDir: true})
+ }
- for _, entry := range entries {
- info := &memFileInfo{name: filepath.Base(entry.path), size: entry.size}
- if err := fn(entry.path, fs.FileInfoToDirEntry(info), nil); err != nil {
+ paths := make([]string, 0, len(entriesByPath))
+ for entryPath := range entriesByPath {
+ paths = append(paths, entryPath)
+ }
+ sort.Strings(paths)
Review Comment:
Sorting the full URI strings doesn't reproduce `fs.WalkDir`'s traversal order, and I think that's worth deciding on deliberately rather than by accident.
Because `.` (0x2E) sorts before `/` (0x2F), a sibling file can land between a directory and its children. With `root/a/child.txt` and `root/a.txt`, `sort.Strings` gives `root/a`, `root/a.txt`, `root/a/child.txt` — but real pre-order traversal visits `root/a`, `root/a/child.txt`, then `root/a.txt`, descending into `a/` before its sibling. The test name (`...EmitsDirectoriesLexically...`) reads like this is intentional, so I want to name the choice: any caller relying on parent-before-all-children ordering (the stdlib contract) would break, even if none in-tree do today.
If we're fine declaring MemFS emits in lexical order and not pre-order, let's say so in the doc comment. Otherwise a recursive descent over the synthesized tree (or a comparator that treats the separator as the primary key) gets us real pre-order. Which contract do we want here?
##########
io/mem_test.go:
##########
@@ -257,5 +262,32 @@ func TestMemIO_WalkDirCallbackCanRemoveFiles(t *testing.T) {
return nil
}))
- assert.Empty(t, remaining)
+ assert.Equal(t, []string{"mem://bucket/root"}, remaining)
+}
+
+func TestMemIO_WalkDirEmitsDirectoriesLexicallyAndHonorsSkipDir(t *testing.T) {
Review Comment:
Nice to see the SkipDir case covered here. A few sibling cases would round it out: SkipAll from mid-walk (the `fs.SkipAll` arm isn't exercised at all), SkipDir when `path == root`, and — most important given the condition above — a root that's also a file key, which is exactly where the `addEntry` overwrite bites.
The nonexistent-root case is worth one too, since that's the LocalFS divergence I flagged. These are all a couple of lines each.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org