SteNicholas commented on code in PR #494:
URL: https://github.com/apache/paimon-cpp/pull/494#discussion_r4444493512
##########
src/paimon/core/manifest/index_manifest_file_handler_test.cpp:
##########
@@ -344,8 +345,43 @@ TEST_F(IndexManifestFileHandlerTest,
GlobalDvCombinerReplacesIndexFileInBucketUn
ASSERT_EQ(written_file_names, (std::set<std::string>{"dv-0-newer",
"dv-1"}));
}
+TEST_F(IndexManifestFileHandlerTest, DynamicBucketDvUsesBucketedCombiner) {
+ ASSERT_OK_AND_ASSIGN(auto index_manifest_file,
CreateManifestFile(BucketMode::HASH_DYNAMIC));
+
+ auto partition = BinaryRow::EmptyRow();
+ std::vector<IndexManifestEntry> previous_entries = {
+ MakeDvEntry(FileKind::Add(), partition, /*bucket=*/0, "dv-0",
{"data-0.orc"}, 1),
+ MakeDvEntry(FileKind::Add(), partition, /*bucket=*/1, "dv-1",
{"data-1.orc"}, 1)};
+ ASSERT_OK_AND_ASSIGN(std::string previous_manifest,
+ IndexManifestFileHandler::Write(
+ /*previous_index_manifest=*/std::nullopt,
previous_entries,
+ BucketMode::HASH_DYNAMIC,
index_manifest_file.get()));
+
+ // a dynamic bucket table rewrites the whole index file of a bucket, so
the added entry
+ // replaces dv-0 by (partition, bucket) although it is not deleted by name
+ std::vector<IndexManifestEntry> new_entries = {MakeDvEntry(
+ FileKind::Add(), partition, /*bucket=*/0, "dv-0-new", {"data-0.orc",
"data-2.orc"}, 2)};
+ ASSERT_OK_AND_ASSIGN(
+ std::string current_manifest,
+ IndexManifestFileHandler::Write(previous_manifest, new_entries,
BucketMode::HASH_DYNAMIC,
Review Comment:
Could this regression test exercise `IndexManifestFile::WriteIndexFiles()`?
Calling the handler directly with `BucketMode::HASH_DYNAMIC` verifies combiner
selection, but bypasses the `Create()` → `bucket_mode_` → `WriteIndexFiles()`
propagation changed by this PR. The test would still pass if the manifest
stored or forwarded the wrong mode. Using the manifest entry point for the
initial write and replacement would cover that path too. For the bucket-unaware
negative case, a separately configured manifest and `ASSERT_NOK_WITH_MSG` would
also ensure rejection happens for the intended duplicate-data-file reason.
##########
src/paimon/core/index/index_file_handler_test.cpp:
##########
@@ -81,7 +81,9 @@ class IndexFileHandlerTest : public testing::Test {
IndexManifestFile::Create(
core_options.GetFileSystem(),
core_options.GetManifestFormat(),
core_options.GetManifestCompression(),
path_factory,
- core_options.GetBucket(), memory_pool_,
core_options));
+ core_options.GetBucket() == -1 ?
BucketMode::BUCKET_UNAWARE
Review Comment:
Could we use `ResolveBucketMode(core_options.GetBucket(), table_schema)`
here as well? This helper already loads `table_schema`, but the new ternary
retains the old assumption that `bucket == -1` always means `BUCKET_UNAWARE`;
it would misclassify a dynamic-bucket primary-key fixture. The current
read-only tests do not expose the difference, but reusing the resolver would
keep this helper consistent with production callers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
