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.
|
|
||
| Result<std::unique_ptr<OutputStream>> LocalFileSystem::Create(const std::string& path, | ||
| bool overwrite) const { | ||
| PAIMON_ASSIGN_OR_RAISE(bool is_exist, Exists(path)); |
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.
| PAIMON_ASSIGN_OR_RAISE(LocalFile dst_file, ToFile(dst)); | ||
| auto parent = dst_file.GetParentFile(); | ||
| PAIMON_RETURN_NOT_OK(Mkdirs(parent.GetAbsolutePath())); | ||
| if (::rename(src.c_str(), dst.c_str()) != 0) { |
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).
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