feat(fs): introduce local file system - #41
Conversation
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
I found a correctness blocker in local file creation: creating a path whose parent does not exist currently fails before it reaches the recursive Mkdirs call. Please fix this before merging.
There was a problem hiding this comment.
This calls Exists(path) before creating the parent directories. For a new file under a non-existing parent (for example /tmp/a/b/file), ToFile(path).Exists() returns an IOError because access() fails with ENOENT on the missing parent path, so Create returns before lines 68-71 can create the parent directory and open the output stream. The Create path should treat missing targets as non-existing, then create parents before opening the file; otherwise FileSystem::Create only works when the parent directory already exists.
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Thanks for pointing out my previous review mistake: access(..., F_OK) returns ENOENT when a parent directory is missing, so Create can continue to Mkdirs(parent). I re-reviewed the PR and found a different blocker in Rename: it validates normalized local paths but calls ::rename with the original URI strings.
There was a problem hiding this comment.
This should use the normalized local paths, not the original src/dst strings. Earlier in this method, Exists(src), Exists(dst), ToFile(src), and ToFile(dst) all strip the optional file: scheme via ToFile, but the actual syscall still passes the raw URI strings to ::rename. So Rename("file:/tmp/a", "file:/tmp/b") validates /tmp/a and creates /tmp, then tries to rename a literal path named file:/tmp/a, which fails. Please call ::rename(src_file.GetAbsolutePath().c_str(), dst_file.GetAbsolutePath().c_str()) (or equivalent normalized paths).
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Reviewed the latest update. The previous Rename blocker is resolved: Rename now operates on normalized local paths, and I did not find any new blocking issue in this revision.
2 participants
Purpose
Linked issue: None
Introduce the local file system implementation migrated from the Alibaba Paimon C++ repository.
This PR adds:
LocalFileSysteminput/output stream supportThe requested migration scope was
src/paimon/fs/localexcludingCMakeLists.txt. No additional dependency files were required.License handling:
LICENSE/NOTICE; no third-party declaration updates were required for these files.External contributor handling:
Co-authored-bytrailer or PR thank-you comment is needed.Tests
API and Format
This adds local file system implementation classes under
src/paimon/fs/local. It does not change public storage format or protocol.Documentation
No documentation changes.
Generative AI tooling
Migrated-by: OpenAI Codex