badalprasadsingh commented on code in PR #2205:
URL: https://github.com/apache/iceberg-go/pull/2205#discussion_r4311694195
##########
io/mem_test.go:
##########
@@ -353,3 +353,39 @@ func TestMemIO_WalkDirHonorsSkipAtRootAndSkipAll(t
*testing.T) {
assert.Equal(t, []string{"mem://bucket/root"}, walked)
}
}
+
+func TestMemIO_WalkDirNestedPrefixes(t *testing.T) {
+ memIO := icebergio.NewMemFS()
+ for _, name := range []string{
+ "mem://bucket/root/a/b/c/deep.parquet",
+ "mem://bucket/root/a/first.parquet",
+ "mem://bucket/root/ab/second.parquet",
+ } {
+ require.NoError(t, memIO.WriteFile(name, nil))
+ }
Review Comment:
P2: The PR changes where file sizes are set. But, every file here is written
with nil, so a dropped or misplaced size would still pass. No test checks the
sizes `MemFS.WalkDir` returns, and orphan cleanup reads them through `d.Info()`.
Could we write files with different content lengths, add a size int64 field
to `visitedEntry` filled from `d.Info()`, and assert the sizes on the file
entries?
##########
io/mem_bench_test.go:
##########
@@ -48,3 +49,31 @@ func BenchmarkMemFSWalkDir(b *testing.B) {
})
}
}
+
+func BenchmarkMemFSWalkDirDeep(b *testing.B) {
+ for _, fileCount := range []int{100, 1_000, 5_000} {
+ b.Run(fmt.Sprintf("files=%d", fileCount), func(b *testing.B) {
+ memIO := icebergio.NewMemFS()
Review Comment:
P2: This repeats `BenchmarkMemFSWalkDir` line for line. Only the file name
differs.
We should move the shared body into a helper that takes a name builder.
##########
io/mem_bench_test.go:
##########
@@ -48,3 +49,31 @@ func BenchmarkMemFSWalkDir(b *testing.B) {
})
}
}
+
+func BenchmarkMemFSWalkDirDeep(b *testing.B) {
+ for _, fileCount := range []int{100, 1_000, 5_000} {
+ b.Run(fmt.Sprintf("files=%d", fileCount), func(b *testing.B) {
+ memIO := icebergio.NewMemFS()
+ for i := range fileCount {
+ parts := make([]string, 8)
+ for j := range parts {
+ parts[j] = fmt.Sprintf("dir-%03d",
(i+j*13)%128)
+ }
+ name :=
fmt.Sprintf("mem://bucket/root/%s/file-%05d.parquet", strings.Join(parts, "/"),
i)
+ if err := memIO.WriteFile(name, nil); err !=
nil {
+ b.Fatal(err)
+ }
+ }
+
+ b.ReportAllocs()
+ b.ResetTimer()
Review Comment:
nit: As we are touching this: `b.ResetTimer()` can go: `b.Loop()` already
resets the timer on its first call.
The existing benchmark has the same thing, so the shared helper should fix
both.
##########
io/mem.go:
##########
@@ -136,18 +136,17 @@ func (m *MemFS) WalkDir(root string, fn fs.WalkDirFunc)
error {
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))
+ pathBase, prefix := relative, 0
+ if root != "" {
+ pathBase = key
+ prefix = len(key) - len(relative)
+ }
Review Comment:
nit: pathBase reads like path.Base, which this function also uses, and
`prefix` is an int next to the string `skipPrefix`. It also isn't obvious why
an empty root slices relative instead of key: switching to key would make
`WalkDir("/")` over `/a/b` report `/a/b` instead of today's `a/b`.
I'd rename them (e.g. `filePath` and `start`) and add a one-line comment
saying why.
--
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]
