Skip to content

feat(fs): introduce file system tests#42

Merged
leaves12138 merged 1 commit into
apache:mainfrom
zjw1111:migrate/fs-tests
Jun 2, 2026
Merged

feat(fs): introduce file system tests#42
leaves12138 merged 1 commit into
apache:mainfrom
zjw1111:migrate/fs-tests

Conversation

@zjw1111
Copy link
Copy Markdown
Contributor

@zjw1111 zjw1111 commented Jun 2, 2026

Purpose

Linked issue: None

Introduce file system test coverage migrated from the Alibaba Paimon C++ repository.

This PR adds:

  • FileSystem unit coverage
  • ResolvingFileSystem unit coverage
  • ExternalPathProvider and its unit coverage

The requested migration scope is limited to the test files under src/paimon/common/fs plus external_path_provider.h. Local file system implementation dependencies are intentionally kept out of this PR and are covered by #41.

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

  • git diff --check

API and Format

This adds ExternalPathProvider under src/paimon/common/fs for external data path selection. 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 standalone build blocker: this PR is based on current main, but the new tests include local file system headers that are only introduced by PR #41 and are not present in this PR's base. Please either rebase this PR onto the local file system PR after it is ready/merged, or make this PR self-contained before merging.

#include "paimon/factories/factory_creator.h"
#include "paimon/fs/file_system.h"
#include "paimon/fs/file_system_factory.h"
#include "paimon/fs/local/local_file_system_factory.h"
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 header does not exist on this PR's base (main at 445feeb...) or in this PR's diff; it is introduced by PR #41. As a result, PR #42 is not buildable as a standalone PR against its current base. Please either stack/rebase this PR onto #41 after that dependency is ready, or include/remove the dependency here.

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 re-reviewed PR #42. Apart from the known dependency on the local file system headers from PR #41, I did not find other blockers in this test/provider migration.

@leaves12138 leaves12138 merged commit 54aee69 into apache:main Jun 2, 2026
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