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:
[P3] 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/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:
[P3] Could the comment distinguish the merge key from the consistency check?
`GlobalDeletionVectorCombiner` merges entries by **index file name** and uses
**data file names** to reject duplicate deletion-vector coverage. Saying it
combines by data file name obscures those different roles. Also, "per
(partition, bucket)" would describe the bucketed identity more precisely than
"per bucket".
##########
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:
[P3] 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]
