Skip to content

Commit a22fe13

Browse files
authored
Do not create empty .vac files in metadata consolidation (#5453)
When we are trying to consolidate array or group metadata but there are no array or group metadata present, we are creating an empty `.vac` file on local filesystems. In object stores, notably on S3 that I have tested, our `write` method currently doesn't create a file when size is `0` (this is most probably another issue to fix), so we don't observe the same behavior. This PR unifies the behavior by enforcing that we don't create a `.vac` file if no consolidation is possible, aka if there are 0 or 1 array/group metadata present. We already do such a check for fragment metadata, fragments and commit consolidation. Both tests in this PR were failing without the fix and are now passing. --- TYPE: BUG DESC: Do not create empty .vac files in metadata consolidation
1 parent 3247973 commit a22fe13

File tree

4 files changed

+71
-3
lines changed

4 files changed

+71
-3
lines changed

test/src/unit-capi-consolidation.cc

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7895,3 +7895,48 @@ TEST_CASE_METHOD(
78957895
remove_sparse_array();
78967896
}
78977897
}
7898+
7899+
TEST_CASE_METHOD(
7900+
ConsolidationFx,
7901+
"C API: Test consolidating empty array metadata",
7902+
"[capi][consolidation][array-meta][empty][sc-62900][non-rest]") {
7903+
remove_dense_array();
7904+
create_dense_array();
7905+
7906+
SECTION("Consolidate empty array metadata") {
7907+
// Configuration for consolidating array metadata
7908+
tiledb_config_t* config = nullptr;
7909+
tiledb_error_t* error = nullptr;
7910+
REQUIRE(tiledb_config_alloc(&config, &error) == TILEDB_OK);
7911+
REQUIRE(error == nullptr);
7912+
int rc = tiledb_config_set(
7913+
config, "sm.consolidation.mode", "array_meta", &error);
7914+
REQUIRE(rc == TILEDB_OK);
7915+
REQUIRE(error == nullptr);
7916+
7917+
// Consolidate without having added any array meta
7918+
rc = tiledb_array_consolidate(ctx_, dense_array_uri_.c_str(), config);
7919+
CHECK(rc == TILEDB_OK);
7920+
7921+
// We must have 0 vacuum files after consolidation.
7922+
std::vector<std::string> array_meta_vac_files;
7923+
get_array_meta_vac_files_dense(array_meta_vac_files);
7924+
CHECK(array_meta_vac_files.size() == 0);
7925+
7926+
tiledb_array_t* array;
7927+
rc = tiledb_array_alloc(ctx_, dense_array_uri_.c_str(), &array);
7928+
REQUIRE(rc == TILEDB_OK);
7929+
7930+
// Open for Delete, so that array metadata are loaded from disk, and check
7931+
// there is no failure
7932+
rc = tiledb_array_open(ctx_, array, TILEDB_DELETE);
7933+
REQUIRE(rc == TILEDB_OK);
7934+
7935+
// Vacuum consolidated array metadata (empty) and check there is no failure
7936+
rc = tiledb_config_set(config, "sm.vacuum.mode", "array_meta", &error);
7937+
REQUIRE(rc == TILEDB_OK);
7938+
REQUIRE(error == nullptr);
7939+
rc = tiledb_array_vacuum(ctx_, dense_array_uri_.c_str(), config);
7940+
CHECK(rc == TILEDB_OK);
7941+
}
7942+
}

test/src/unit-capi-group.cc

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1083,6 +1083,19 @@ TEST_CASE_METHOD(
10831083
tiledb::sm::URI group_uri(temp_dir + "group");
10841084
REQUIRE(tiledb_group_create(ctx_, group_uri.c_str()) == TILEDB_OK);
10851085

1086+
// Consolidate empty metadata
1087+
tiledb_config_t* cfg;
1088+
tiledb_error_t* err = nullptr;
1089+
REQUIRE(tiledb_config_alloc(&cfg, &err) == TILEDB_OK);
1090+
int rc = tiledb_group_consolidate_metadata(ctx_, group_uri.c_str(), cfg);
1091+
REQUIRE(rc == TILEDB_OK);
1092+
1093+
// Check no .vac file is created
1094+
std::vector<std::string> group_empty_meta_vac_files;
1095+
get_meta_vac_files(group_uri, group_empty_meta_vac_files);
1096+
CHECK(group_empty_meta_vac_files.size() == 0);
1097+
tiledb_config_free(&cfg);
1098+
10861099
write_group_metadata(group_uri.c_str());
10871100
std::this_thread::sleep_for(std::chrono::milliseconds(1));
10881101
auto start = tiledb::sm::utils::time::timestamp_now_ms();
@@ -1100,7 +1113,7 @@ TEST_CASE_METHOD(
11001113
CHECK(group_meta_files.size() == 4);
11011114

11021115
uint64_t file_size = 0;
1103-
int rc = tiledb_vfs_file_size(
1116+
rc = tiledb_vfs_file_size(
11041117
ctx_, vfs_, group_meta_vac_files[0].c_str(), &file_size);
11051118
REQUIRE(rc == TILEDB_OK);
11061119

tiledb/sm/consolidator/array_meta_consolidator.cc

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,12 @@ Status ArrayMetaConsolidator::consolidate(
8080
encryption_key,
8181
key_length));
8282

83+
// Check if there's actually more than 1 file to consolidate
84+
auto& metadata_r = array_for_reads.metadata();
85+
if (metadata_r.loaded_metadata_uris().size() <= 1) {
86+
return Status::Ok();
87+
}
88+
8389
// Open array for writing
8490
Array array_for_writes(resources_, array_uri);
8591
RETURN_NOT_OK_ELSE(
@@ -88,7 +94,6 @@ Status ArrayMetaConsolidator::consolidate(
8894
throw_if_not_ok(array_for_reads.close()));
8995

9096
// Copy-assign the read metadata into the metadata of the array for writes
91-
auto& metadata_r = array_for_reads.metadata();
9297
array_for_writes.opened_array()->metadata() = metadata_r;
9398
URI new_uri = metadata_r.get_uri(array_uri);
9499
const auto& to_vacuum = metadata_r.loaded_metadata_uris();

tiledb/sm/consolidator/group_meta_consolidator.cc

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,13 @@ Status GroupMetaConsolidator::consolidate(
8383
group_for_reads.close();
8484
}
8585

86-
// Copy-assign the read metadata into the metadata of the group for writes
86+
// Check if there's actually more than 1 file to consolidate
8787
auto metadata_r = group_for_reads.metadata();
88+
if (metadata_r->loaded_metadata_uris().size() <= 1) {
89+
return Status::Ok();
90+
}
91+
92+
// Copy-assign the read metadata into the metadata of the group for writes
8893
*(group_for_writes.metadata()) = *metadata_r;
8994
URI new_uri = metadata_r->get_uri(group_uri);
9095
const auto& to_vacuum = metadata_r->loaded_metadata_uris();

0 commit comments

Comments
 (0)