Skip to content

feat(fs): introduce local file system#41

Open
zjw1111 wants to merge 1 commit into
apache:mainfrom
zjw1111:migrate/fs-local
Open

feat(fs): introduce local file system#41
zjw1111 wants to merge 1 commit into
apache:mainfrom
zjw1111:migrate/fs-local

Conversation

@zjw1111
Copy link
Copy Markdown
Contributor

@zjw1111 zjw1111 commented Jun 2, 2026

Purpose

Linked issue: None

Introduce the local file system implementation migrated from the Alibaba Paimon C++ repository.

This PR adds:

  • local file primitives and file status wrappers
  • LocalFileSystem input/output stream support
  • local file system factory registration
  • local file unit coverage

The requested migration scope was src/paimon/fs/local excluding CMakeLists.txt. No additional dependency files were required.

License handling:

  • Replaced Alibaba-owned source headers with ASF headers.
  • Checked source and target LICENSE / NOTICE; no third-party declaration updates were required for these files.

External contributor handling:

  • Re-ran blame-based contributor analysis for the final file set.
  • No external contributor thresholds were hit, so no Co-authored-by trailer 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

Copy link
Copy Markdown
Contributor

@leaves12138 leaves12138 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@leaves12138 leaves12138 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants