fastio commented on code in PR #494:
URL: https://github.com/apache/paimon-cpp/pull/494#discussion_r4450463138
##########
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:
Done, the test now goes through `WriteIndexFiles()` and asserts the exact
duplicate-vector error on a separate `BUCKET_UNAWARE` manifest.
##########
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:
Done, switched to `ResolveBucketMode(core_options.GetBucket(),
table_schema)`.
##########
src/paimon/core/manifest/index_manifest_file_handler.cpp:
##########
@@ -255,17 +255,14 @@ IndexManifestFileHandler::SeparateIndexEntries(
Result<std::unique_ptr<IndexManifestFileHandler::IndexManifestFileCombiner>>
IndexManifestFileHandler::GetIndexManifestFileCombine(const std::string&
index_type,
- int32_t bucket_mode) {
+ BucketMode bucket_mode) {
if (index_type != DeletionVectorsIndexFile::DELETION_VECTORS_INDEX &&
index_type != "HASH") {
return std::make_unique<GlobalFileNameCombiner>();
}
- // `bucket_mode` is the configured bucket, not a resolved BucketMode,
standing in for Paimon
- // Java's BucketMode.BUCKET_UNAWARE check. The two agree on every table
that can exist:
- // SchemaValidation rejects the other unaware bucket, 0, and
BucketIdCalculator refuses to
- // write the primary key table on which -1 means HASH_DYNAMIC instead.
Lifting either
- // restriction means passing the resolved BucketMode here, or a dynamic
bucket table would
- // combine its per-bucket deletion vectors by index file name.
- if (index_type == DeletionVectorsIndexFile::DELETION_VECTORS_INDEX &&
bucket_mode == -1) {
+ // A dynamic bucket primary key table also has bucket=-1, but keeps one
deletion vector index
+ // per bucket, so only BUCKET_UNAWARE combines deletion vectors by data
file name.
Review Comment:
Done, the comment now separates merging by index file name from the
data-file duplicate check and says "per (partition, bucket)".
--
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]
