Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix address sanitizer error when storing metadata in destructor #473

Merged
merged 4 commits into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions src/include/index/index_group.h
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,6 @@ class base_index_group {
"Cannot clear history because group does not exist.");
}

metadata_.clear_history(timestamp);
try {
tiledb::Array::delete_fragments(cached_ctx_, ids_uri(), 0, timestamp);
static_cast<group_type*>(this)->clear_history_impl(timestamp);
Expand All @@ -374,19 +373,23 @@ class base_index_group {
}
throw e;
}

metadata_.clear_history(timestamp);
store_metadata();
}

/**
* @brief Destructor. If opened for write, update the metadata.
*
* @todo Don't use default Config
*/
~base_index_group() {
if (opened_for_ == TILEDB_WRITE && exists()) {
auto write_group = tiledb::Group(
cached_ctx_, group_uri_, TILEDB_WRITE, cached_ctx_.config());
metadata_.store_metadata(write_group);
void store_metadata() {
if (opened_for_ == TILEDB_READ) {
throw std::runtime_error(
"[index_group@write] Cannot write in read mode.");
}
if (!exists()) {
throw std::runtime_error(
"[index_group@write] Cannot write because group does not exist.");
}
auto write_group = tiledb::Group(
cached_ctx_, group_uri_, TILEDB_WRITE, cached_ctx_.config());
metadata_.store_metadata(write_group);
}

/**
Expand Down
3 changes: 1 addition & 2 deletions src/include/index/ivf_flat_index.h
Original file line number Diff line number Diff line change
Expand Up @@ -477,12 +477,11 @@ class ivf_flat_index {
temporal_policy_,
storage_version,
dimensions_);

write_group.set_dimensions(dimensions_);

write_group.append_ingestion_timestamp(temporal_policy_.timestamp_end());
write_group.append_base_size(::num_vectors(*partitioned_vectors_));
write_group.append_num_partitions(num_partitions_);
write_group.store_metadata();

write_matrix(
ctx,
Expand Down
3 changes: 2 additions & 1 deletion src/include/index/ivf_pq_index.h
Original file line number Diff line number Diff line change
Expand Up @@ -1102,7 +1102,6 @@ class ivf_pq_index {
dimensions_,
num_clusters_,
num_subspaces_);

write_group.set_dimensions(dimensions_);
write_group.set_num_subspaces(num_subspaces_);
write_group.set_sub_dimensions(sub_dimensions_);
Expand Down Expand Up @@ -1150,6 +1149,8 @@ class ivf_pq_index {
write_group.append_num_partitions(num_partitions_);
}

write_group.store_metadata();

// When creating from Python we initially call write_index() at timestamp 0.
// The goal here is just to create the arrays and save metadata. Return here
// so that we don't write the arrays, as if we write with timestamp=0 then
Expand Down
2 changes: 2 additions & 0 deletions src/include/index/vamana_index.h
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,8 @@ class vamana_index {
write_group.append_num_edges(graph_.num_edges());
}

write_group.store_metadata();

// When creating from Python we initially call write_index() at timestamp 0.
// The goal here is just to create the arrays and save metadata. Return here
// so that we don't write the arrays, as if we write with timestamp=0 then
Expand Down
9 changes: 5 additions & 4 deletions src/include/test/unit_ivf_pq_group.cc
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,12 @@ TEST_CASE("write constructor - create and read", "[ivf_pq_group]") {
x.append_base_size(0);
x.append_ingestion_timestamp(0);

// We throw b/c the ivf_pq_group hasn't actually been written (the
// write happens in the destructor).
// We throw b/c the ivf_pq_group hasn't actually been stored yet.
CHECK_THROWS_WITH(
ivf_pq_group<dummy_index>(ctx, tmp_uri, TILEDB_READ),
"No ingestion timestamps found.");

x.store_metadata();
}

ivf_pq_group y = ivf_pq_group<dummy_index>(ctx, tmp_uri, TILEDB_READ);
Expand Down Expand Up @@ -168,8 +169,7 @@ TEST_CASE("write constructor - invalid create and read", "[ivf_pq_group]") {
CHECK(x.get_dimensions() == dimensions);
CHECK(x.get_num_clusters() == num_clusters);
CHECK(x.get_num_subspaces() == num_subspaces);
// We throw b/c the ivf_pq_group hasn't actually been written (the
// write happens in the destructor).
// We throw b/c the ivf_pq_group hasn't actually been stored yet.
CHECK_THROWS_WITH(
ivf_pq_group<dummy_index>(ctx, tmp_uri, TILEDB_READ),
"No ingestion timestamps found.");
Expand Down Expand Up @@ -216,6 +216,7 @@ TEST_CASE("group metadata - bases, ingestions, partitions", "[ivf_pq_group]") {
x.append_num_partitions(0);
x.append_base_size(0);
x.append_ingestion_timestamp(0);
x.store_metadata();
}

ivf_pq_group x = ivf_pq_group<dummy_index>(
Expand Down
9 changes: 5 additions & 4 deletions src/include/test/unit_vamana_group.cc
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,12 @@ TEST_CASE("write constructor - create and read", "[vamana_group]") {
x.append_base_size(0);
x.append_ingestion_timestamp(0);

// We throw b/c the vamana_index_group hasn't actually been written (the
// write happens in the destructor).
// We throw b/c the vamana_index_group hasn't actually been stored yet.
CHECK_THROWS_WITH(
vamana_index_group<dummy_index>(ctx, tmp_uri, TILEDB_READ),
"No ingestion timestamps found.");

x.store_metadata();
}

vamana_index_group y =
Expand All @@ -139,8 +140,7 @@ TEST_CASE("write constructor - invalid create and read", "[vamana_group]") {
vamana_index_group x =
vamana_index_group<dummy_index>(ctx, tmp_uri, TILEDB_WRITE, {}, "", 10);
CHECK(x.get_dimensions() == 10);
// We throw b/c the vamana_index_group hasn't actually been written (the
// write happens in the destructor).
// We throw b/c the vamana_index_group hasn't actually been stored yet.
CHECK_THROWS_WITH(
vamana_index_group<dummy_index>(ctx, tmp_uri, TILEDB_READ),
"No ingestion timestamps found.");
Expand Down Expand Up @@ -179,6 +179,7 @@ TEST_CASE("group metadata - bases, ingestions, partitions", "[vamana_group]") {
x.append_num_edges(0);
x.append_base_size(0);
x.append_ingestion_timestamp(0);
x.store_metadata();
}

vamana_index_group x =
Expand Down
Loading