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: [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]
