feat(fs): introduce file system abstractions - #26
Conversation
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Thanks for migrating the file system abstractions. I found a blocking build/link issue in the current head.
Several newly added utility APIs are only declared, but no implementation is added in this PR. For example:
Path::ToString,PathUtil::CreateTempPath, andPathUtil::ToPathare declared insrc/paimon/common/utils/path_util.h, and are called by the new file system code.StringUtils::StartsWith,StringUtils::EndsWith, andStringUtils::ToLowerCaseare declared insrc/paimon/common/utils/string_utils.h, and existing/new code calls them fromdata_type_json_parser.cpp,blob_utils.cpp, andfile_system.cpp.
I could not find any corresponding definitions in the PR. This means the code either fails to link as soon as those call sites are included in a target, or fails to build if these utilities are expected to be header-only. Please add the missing definitions (or inline the functions in the headers) and include tests/at least a build target that exercises the new file system and utility code.
I also noticed that this PR does not add any tests for the new path parsing / resolving file system behavior, so the cache key, default scheme mapping, and helper functions are currently unverified.
zjw1111
changed the title
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Thanks for the update. Deferring tests to a follow-up PR is acceptable, but the current head still has a blocking build issue.
The final PR head no longer contains the utility files from the intermediate commit, but the new file system code still includes and calls them:
src/paimon/common/fs/file_system.cppincludespaimon/common/utils/path_util.handpaimon/common/utils/string_utils.h, and callsPathUtil::ToPath,StringUtils::ToLowerCase, andPathUtil::CreateTempPath.src/paimon/common/fs/resolving_file_system.cppincludespaimon/common/utils/path_util.hand callsPathUtil::ToPath.
Those headers are not present in the final head tree, so this PR cannot compile as-is. Please either keep the needed utility headers/implementations in this PR, or remove/replace these dependencies before merging.
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Re-reviewed against the current merge result after PR #23. The utility headers and implementations (path_util, string_utils, and related helpers) are present on main, so my previous build-blocker comment was based on the stale PR base/head view and is no longer valid.
The file system abstraction changes look good to me. Deferring the tests to the follow-up PR is acceptable for this split.
2 participants
Purpose
Linked issue: None
Migrate the basic file system abstractions from the Alibaba-origin C++ repository into Apache Paimon C++.
Requested files migrated:
include/paimon/fs/file_system.hinclude/paimon/fs/file_system_factory.hsrc/paimon/common/fs/file_system.cppsrc/paimon/common/fs/file_system_factory.cppsrc/paimon/common/fs/resolving_file_system.hsrc/paimon/common/fs/resolving_file_system.cppExtra dependency files migrated for a complete build closure:
include/paimon/factories/factory.hinclude/paimon/factories/factory_creator.hsrc/paimon/common/factories/factory_creator.cppinclude/paimon/factories/singleton.hsrc/paimon/common/factories/singleton.cppsrc/paimon/common/factories/io_hook.hsrc/paimon/common/factories/io_hook.cppLicense handling:
singleton.handsingleton.cpp.LICENSEandNOTICE.External contributor handling:
Co-authored-bytrailer was added.Tests
git diff --checkclang-formaton migrated C++ filesNote:
cmake --build build -j64could not run in this checkout because the existingbuild/directory has no generatedMakefileor other usable build generator files.API and Format
This adds public file system and factory headers under
include/paimon/. It does not change storage format or protocol.Documentation
No user-facing documentation changes.
Generative AI tooling
Migrate-by: OpenAI Codex