Skip to content

test(spill): add unit tests and integration tests for spill-to-disk#272

Open
dalingmeng wants to merge 1 commit into
alibaba:mainfrom
dalingmeng:feat/spill-test
Open

test(spill): add unit tests and integration tests for spill-to-disk#272
dalingmeng wants to merge 1 commit into
alibaba:mainfrom
dalingmeng:feat/spill-test

Conversation

@dalingmeng
Copy link
Copy Markdown
Contributor

@dalingmeng dalingmeng commented May 11, 2026

Purpose

Linked issue: #149
add unit tests and integration tests for spill-to-disk

Tests

WriteInteTest.TestPkSpillable.*
KeyValueFileStoreWriteTest.TestSpill.*
MergeTreeWriterTest.TestSpill.*
WriteBufferTest.TestSpill.*
SpillReaderWriterTest

API and Format

Documentation

Generative AI tooling

Claude opus 4.6

Comment thread src/paimon/core/mergetree/merge_tree_writer_test.cpp
Comment thread src/paimon/core/mergetree/merge_tree_writer_test.cpp
Comment thread src/paimon/core/mergetree/merge_tree_writer_test.cpp
Comment thread src/paimon/core/mergetree/write_buffer_test.cpp
Comment thread src/paimon/core/mergetree/write_buffer_test.cpp
Comment thread test/inte/write_inte_test.cpp Outdated
Comment thread test/inte/write_inte_test.cpp
@lxy-9602 lxy-9602 changed the title test(spill): add unit tests and integration tests for spill-to-disk) test(spill): add unit tests and integration tests for spill-to-disk May 12, 2026
@dalingmeng dalingmeng requested a review from lxy-9602 May 12, 2026 10:39
@zjw1111 zjw1111 requested a review from Copilot May 12, 2026 10:40
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds comprehensive unit/integration coverage for spill-to-disk behavior (incl. quota exhaustion, cleanup, dedup, intermediate merge, and IO error handling), linked to issue #149.

Changes:

  • Adds new spill-focused integration tests to validate correctness, cleanup, and failure fallbacks.
  • Adds unit tests around spill file quotas, reader ordering, merge behavior, and resource cleanup.
  • Refactors repeated commit/scan logic in integration tests into helper methods.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
test/inte/write_inte_test.cpp Adds PK spill integration tests + introduces Commit/Scan helper utilities to reduce duplication.
src/paimon/core/operation/key_value_file_store_write_test.cpp Adds spill tests for disk-quota fallback and multi-round dedup semantics.
src/paimon/core/mergetree/write_buffer_test.cpp Adds extensive WriteBuffer spill/quota/reader/cleanup test coverage.
src/paimon/core/mergetree/spill_reader_writer_test.cpp Adds tests for writer Close idempotency and schema mismatch error cases.
src/paimon/core/mergetree/merge_tree_writer_test.cpp Adds MergeTreeWriter spill tests: dedup, intermediate merge bounds, quota fallback, cleanup.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +251 to +263
Status CommitMessages(const std::string& table_path,
const std::vector<std::shared_ptr<CommitMessage>>& commit_messages,
const std::map<std::string, std::string>& commit_options,
bool ignore_empty_commit = true,
int64_t commit_identifier = BATCH_WRITE_COMMIT_IDENTIFIER) const {
CommitContextBuilder commit_builder(table_path, "commit_user_1");
commit_builder.SetOptions(commit_options);
commit_builder.IgnoreEmptyCommit(ignore_empty_commit);
PAIMON_ASSIGN_OR_RAISE(auto commit_context, commit_builder.Finish());
PAIMON_ASSIGN_OR_RAISE(auto file_store_commit,
FileStoreCommit::Create(std::move(commit_context)));
return file_store_commit->Commit(commit_messages, commit_identifier);
}
ASSERT_TRUE(success_b1);
}

TEST_P(WriteInteTest, TestPkSpillableWithIOException) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

unnecessary

schema_manger.ReadSchema(/*schema_id=*/0));
DataGenerator gen(table_schema, pool_);

for (size_t i = 0; i < 2000; i++) {
Comment on lines +4098 to +4103
for (const auto& split : data_splits) {
auto split_impl = dynamic_cast<DataSplitImpl*>(split.get());
ASSERT_OK_AND_ASSIGN(std::string partition_str,
helper->PartitionStr(split_impl->Partition()));
splits_by_partition[partition_str].push_back(split);
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

unnecessary

Comment thread src/paimon/core/operation/key_value_file_store_write_test.cpp
Comment thread src/paimon/core/mergetree/spill_reader_writer_test.cpp
@dalingmeng dalingmeng force-pushed the feat/spill-test branch 2 times, most recently from a035943 to 3a18972 Compare May 13, 2026 09:23
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.

3 participants