From 431092e460ea44c9ba348215eaab0842016d064b Mon Sep 17 00:00:00 2001 From: Zhang Jiawei <30893610+zjw1111@users.noreply.github.com> Date: Thu, 26 Feb 2026 14:03:59 +0800 Subject: [PATCH] fix: fix some disabled ut in FileStoreCommitImplTest (#148) --- README.md | 10 +++ docs/source/building.rst | 1 + .../string_to_timestamp_cast_executor.cpp | 3 +- .../core/catalog/renaming_snapshot_commit.h | 3 +- .../operation/file_store_commit_impl_test.cpp | 70 ++++++++----------- src/paimon/core/utils/tag_manager.cpp | 3 +- 6 files changed, 44 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index a6464950..87a43ba3 100644 --- a/README.md +++ b/README.md @@ -119,6 +119,16 @@ The reading is divided into two stages: ## Development +### Clone the Repository + +If you don't have `git-lfs` installed, please install it first. + +``` +$ git clone https://github.com/alibaba/paimon-cpp.git +$ cd paimon-cpp +$ git lfs pull +``` + ### CMake ``` diff --git a/docs/source/building.rst b/docs/source/building.rst index 9ae95165..6416a91d 100644 --- a/docs/source/building.rst +++ b/docs/source/building.rst @@ -57,6 +57,7 @@ repository: $ git clone https://github.com/alibaba/paimon-cpp.git $ cd paimon-cpp + $ git lfs pull Manual configuration -------------------- diff --git a/src/paimon/core/casting/string_to_timestamp_cast_executor.cpp b/src/paimon/core/casting/string_to_timestamp_cast_executor.cpp index 90b445da..c73bcbc7 100644 --- a/src/paimon/core/casting/string_to_timestamp_cast_executor.cpp +++ b/src/paimon/core/casting/string_to_timestamp_cast_executor.cpp @@ -16,12 +16,11 @@ #include "paimon/core/casting/string_to_timestamp_cast_executor.h" -#include - #include #include #include +#include "arrow/compute/cast.h" #include "arrow/type.h" #include "arrow/util/checked_cast.h" #include "paimon/core/casting/casting_utils.h" diff --git a/src/paimon/core/catalog/renaming_snapshot_commit.h b/src/paimon/core/catalog/renaming_snapshot_commit.h index 5dae8658..5c33c3a0 100644 --- a/src/paimon/core/catalog/renaming_snapshot_commit.h +++ b/src/paimon/core/catalog/renaming_snapshot_commit.h @@ -16,8 +16,6 @@ #pragma once -#include - #include #include #include @@ -26,6 +24,7 @@ #include "paimon/core/catalog/snapshot_commit.h" #include "paimon/core/snapshot.h" #include "paimon/core/utils/snapshot_manager.h" +#include "paimon/fs/file_system.h" #include "paimon/result.h" #include "paimon/status.h" diff --git a/src/paimon/core/operation/file_store_commit_impl_test.cpp b/src/paimon/core/operation/file_store_commit_impl_test.cpp index 60287abd..9cd07801 100644 --- a/src/paimon/core/operation/file_store_commit_impl_test.cpp +++ b/src/paimon/core/operation/file_store_commit_impl_test.cpp @@ -37,7 +37,6 @@ #include "paimon/common/data/binary_row.h" #include "paimon/common/data/binary_row_writer.h" #include "paimon/common/factories/io_hook.h" -#include "paimon/common/fs/resolving_file_system.h" #include "paimon/common/utils/path_util.h" #include "paimon/common/utils/scope_guard.h" #include "paimon/core/catalog/commit_table_request.h" @@ -62,7 +61,6 @@ #include "paimon/fs/local/local_file_system_factory.h" #include "paimon/memory/memory_pool.h" #include "paimon/metrics.h" -#include "paimon/string_builder.h" #include "paimon/testing/utils/binary_row_generator.h" #include "paimon/testing/utils/io_exception_helper.h" #include "paimon/testing/utils/testharness.h" @@ -88,26 +86,27 @@ class GmockFileSystemFactory : public LocalFileSystemFactory { Result> Create( const std::string& path, const std::map& options) const override { - auto fs = std::make_unique(); + auto fs = std::make_unique>(); + auto fs_ptr = fs.get(); using ::testing::A; using ::testing::Invoke; ON_CALL(*fs, ListDir(A(), A>*>())) .WillByDefault( - Invoke([&](const std::string& directory, - std::vector>* file_status_list) { - return fs->LocalFileSystem::ListDir(directory, file_status_list); + Invoke([fs_ptr](const std::string& directory, + std::vector>* file_status_list) { + return fs_ptr->LocalFileSystem::ListDir(directory, file_status_list); })); ON_CALL(*fs, ReadFile(A(), A())) - .WillByDefault(Invoke([&](const std::string& path, std::string* content) { - return fs->FileSystem::ReadFile(path, content); + .WillByDefault(Invoke([fs_ptr](const std::string& path, std::string* content) { + return fs_ptr->FileSystem::ReadFile(path, content); })); ON_CALL(*fs, AtomicStore(A(), A())) - .WillByDefault(Invoke([&](const std::string& path, const std::string& content) { - return fs->FileSystem::AtomicStore(path, content); + .WillByDefault(Invoke([fs_ptr](const std::string& path, const std::string& content) { + return fs_ptr->FileSystem::AtomicStore(path, content); })); return fs; @@ -364,33 +363,31 @@ TEST_F(FileStoreCommitImplTest, TestRESTCatalogCommit) { ASSERT_FALSE(exist); } -// TODO(jinli.zjw): fix disabled case -TEST_F(FileStoreCommitImplTest, DISABLED_TestCommitWithConflictSnapshotAndRetryTenTimes) { +TEST_F(FileStoreCommitImplTest, TestCommitWithConflictSnapshotAndRetryTenTimes) { std::string test_data_path = paimon::test::GetDataDir() + "/orc/append_09.db/append_09/"; auto dir = UniqueTestDirectory::Create(); std::string table_path = dir->Str(); ASSERT_TRUE(TestUtil::CopyDirectory(test_data_path, table_path)); + ASSERT_OK_AND_ASSIGN(std::shared_ptr fs, + FileSystemFactory::Get("gmock_fs", table_path, {})); CommitContextBuilder context_builder(table_path, "commit_user_1"); ASSERT_OK_AND_ASSIGN(std::unique_ptr commit_context, context_builder.AddOption(Options::MANIFEST_FORMAT, "orc") .AddOption(Options::MANIFEST_TARGET_FILE_SIZE, "8mb") .AddOption(Options::COMMIT_MAX_RETRIES, "10") - .AddOption(Options::FILE_SYSTEM, "gmock_fs") + .WithFileSystem(fs) .Finish()); ASSERT_OK_AND_ASSIGN(auto commit, FileStoreCommit::Create(std::move(commit_context))); - auto commit_impl = dynamic_cast(commit.get()); std::string latest_hint = PathUtil::JoinPath(table_path, "snapshot/LATEST"); - auto* resolving_fs = - dynamic_cast(commit_impl->snapshot_manager_->fs_.get()); - ASSERT_OK_AND_ASSIGN(auto real_fs, resolving_fs->GetRealFileSystem(table_path)); - auto* mock_fs = dynamic_cast(real_fs.get()); + auto* mock_fs = dynamic_cast(fs.get()); EXPECT_CALL(*mock_fs, ReadFile(testing::StrEq(latest_hint), testing::_)) .WillRepeatedly(testing::Invoke([](const std::string& path, std::string* content) { *content = "-1"; return Status::OK(); })); + EXPECT_CALL(*mock_fs, ListDir(testing::_, testing::_)).Times(testing::AnyNumber()); EXPECT_CALL(*mock_fs, ListDir(testing::StrEq(PathUtil::JoinPath(table_path, "snapshot")), testing::_)) .WillRepeatedly( @@ -407,25 +404,23 @@ TEST_F(FileStoreCommitImplTest, DISABLED_TestCommitWithConflictSnapshotAndRetryT ASSERT_NOK(commit->Commit(msgs)); } -TEST_F(FileStoreCommitImplTest, DISABLED_TestCommitWithConflictSnapshotAndRetryOnce) { +TEST_F(FileStoreCommitImplTest, TestCommitWithConflictSnapshotAndRetryOnce) { std::string test_data_path = paimon::test::GetDataDir() + "/orc/append_09.db/append_09/"; auto dir = UniqueTestDirectory::Create(); std::string table_path = dir->Str(); ASSERT_TRUE(TestUtil::CopyDirectory(test_data_path, table_path)); + ASSERT_OK_AND_ASSIGN(std::shared_ptr fs, + FileSystemFactory::Get("gmock_fs", table_path, {})); CommitContextBuilder context_builder(table_path, "commit_user_1"); ASSERT_OK_AND_ASSIGN(std::unique_ptr commit_context, context_builder.AddOption(Options::MANIFEST_FORMAT, "orc") .AddOption(Options::MANIFEST_TARGET_FILE_SIZE, "8mb") - .AddOption(Options::FILE_SYSTEM, "gmock_fs") + .WithFileSystem(fs) .Finish()); ASSERT_OK_AND_ASSIGN(auto commit, FileStoreCommit::Create(std::move(commit_context))); - auto commit_impl = dynamic_cast(commit.get()); std::string latest_hint = PathUtil::JoinPath(table_path, "snapshot/LATEST"); - auto* resolving_fs = - dynamic_cast(commit_impl->snapshot_manager_->fs_.get()); - ASSERT_OK_AND_ASSIGN(auto real_fs, resolving_fs->GetRealFileSystem(table_path)); - auto* mock_fs = dynamic_cast(real_fs.get()); + auto* mock_fs = dynamic_cast(fs.get()); EXPECT_CALL(*mock_fs, ReadFile(testing::StrEq(latest_hint), testing::_)) .WillRepeatedly(testing::Invoke([](const std::string& path, std::string* content) { *content = "-1"; @@ -439,6 +434,7 @@ TEST_F(FileStoreCommitImplTest, DISABLED_TestCommitWithConflictSnapshotAndRetryO return mock_fs->FileSystem::ReadFile(path, content); })); + EXPECT_CALL(*mock_fs, ListDir(testing::_, testing::_)).Times(testing::AnyNumber()); EXPECT_CALL(*mock_fs, ListDir(testing::StrEq(PathUtil::JoinPath(table_path, "snapshot")), testing::_)) .WillOnce( @@ -468,25 +464,23 @@ TEST_F(FileStoreCommitImplTest, DISABLED_TestCommitWithConflictSnapshotAndRetryO ASSERT_TRUE(exist); } -TEST_F(FileStoreCommitImplTest, - DISABLED_TestCommitWithAtomicWriteSnapshotTimeoutAndActuallySucceed) { +TEST_F(FileStoreCommitImplTest, TestCommitWithAtomicWriteSnapshotTimeoutAndActuallySucceed) { std::string test_data_path = paimon::test::GetDataDir() + "/orc/append_09.db/append_09/"; auto dir = UniqueTestDirectory::Create(); std::string table_path = dir->Str(); ASSERT_TRUE(TestUtil::CopyDirectory(test_data_path, table_path)); + ASSERT_OK_AND_ASSIGN(std::shared_ptr fs, + FileSystemFactory::Get("gmock_fs", table_path, {})); CommitContextBuilder context_builder(table_path, "commit_user_1"); ASSERT_OK_AND_ASSIGN(std::unique_ptr commit_context, context_builder.AddOption(Options::MANIFEST_FORMAT, "orc") .AddOption(Options::MANIFEST_TARGET_FILE_SIZE, "8mb") - .AddOption(Options::FILE_SYSTEM, "gmock_fs") + .WithFileSystem(fs) .Finish()); ASSERT_OK_AND_ASSIGN(auto commit, FileStoreCommit::Create(std::move(commit_context))); std::string new_snapshot_6 = PathUtil::JoinPath(table_path, "snapshot/snapshot-6"); - auto commit_impl = dynamic_cast(commit.get()); - auto* resolving_fs = dynamic_cast(commit_impl->fs_.get()); - ASSERT_OK_AND_ASSIGN(auto real_fs, resolving_fs->GetRealFileSystem(table_path)); - auto* mock_fs = dynamic_cast(real_fs.get()); + auto* mock_fs = dynamic_cast(fs.get()); EXPECT_CALL(*mock_fs, AtomicStore(testing::StrEq(new_snapshot_6), testing::_)) .WillOnce(testing::Invoke([&](const std::string& path, const std::string& content) { // to mock atomic store timeout actually succeed @@ -507,22 +501,18 @@ TEST_F(FileStoreCommitImplTest, CommitContextBuilder context_builder_2(table_path, "commit_user_1"); ASSERT_OK_AND_ASSIGN(std::unique_ptr commit_context_2, - context_builder.AddOption(Options::MANIFEST_FORMAT, "orc") + context_builder_2.AddOption(Options::MANIFEST_FORMAT, "orc") .AddOption(Options::MANIFEST_TARGET_FILE_SIZE, "8mb") - .AddOption(Options::FILE_SYSTEM, "gmock_fs") + .WithFileSystem(fs) .Finish()); ASSERT_OK_AND_ASSIGN(auto commit_2, FileStoreCommit::Create(std::move(commit_context_2))); ASSERT_OK_AND_ASSIGN(int32_t num_committed, commit_2->FilterAndCommit({{1, msgs}})); ASSERT_EQ(0, num_committed); - auto commit_impl_2 = dynamic_cast(commit_2.get()); - auto* resolving_fs_2 = dynamic_cast(commit_impl_2->fs_.get()); - ASSERT_OK_AND_ASSIGN(auto real_fs_2, resolving_fs_2->GetRealFileSystem(table_path)); - auto* mock_fs_2 = dynamic_cast(real_fs_2.get()); std::string new_snapshot_7 = PathUtil::JoinPath(table_path, "snapshot/snapshot-7"); - EXPECT_CALL(*mock_fs_2, AtomicStore(testing::StrEq(new_snapshot_7), testing::_)) + EXPECT_CALL(*mock_fs, AtomicStore(testing::StrEq(new_snapshot_7), testing::_)) .WillOnce(testing::Invoke([&](const std::string& path, const std::string& content) { - return mock_fs_2->FileSystem::AtomicStore(path, content); + return mock_fs->FileSystem::AtomicStore(path, content); })); std::vector> msgs_2 = GetCommitMessages(paimon::test::GetDataDir() + diff --git a/src/paimon/core/utils/tag_manager.cpp b/src/paimon/core/utils/tag_manager.cpp index b57ff89a..4fc58309 100644 --- a/src/paimon/core/utils/tag_manager.cpp +++ b/src/paimon/core/utils/tag_manager.cpp @@ -16,8 +16,6 @@ #include "paimon/core/utils/tag_manager.h" -#include - #include #include #include @@ -26,6 +24,7 @@ #include "paimon/common/utils/path_util.h" #include "paimon/core/tag/tag.h" #include "paimon/core/utils/branch_manager.h" +#include "paimon/fs/file_system.h" namespace paimon {