Skip to content

Commit a84789e

Browse files
Support reading V1 group details with explicit version in the name. (#4744)
[SC-41275](https://app.shortcut.com/tiledb-inc/story/41275/fix-v1-groups-written-with-latest-version) When version 2 of the group details format was introduced in #3928 (2.16 and also backported to 2.15), the names of group details V2 files were of the format `__t1_t2_id_version`, and V1 kept using `__t1_t2_id` for compatibility. In #4530 (2.19), V1 files started being written with the new format as well. This confused the group reader, which considered group details files with names of the form `__t1_t2_id_1` as V2, merely because they had a version field in their name. This PR fixes the group reader, making it always consider the version if it is present in the name of details file. The change has been validated by writing to a V1 group and then reading from it. The test group was created locally by Core version 2.14 and was added in `test/inputs/groups` (its size is tiny so it can safely be stored in the repository). --- TYPE: BUG DESC: Support reading V1 group details with explicit version in the name.
1 parent c6c7615 commit a84789e

File tree

5 files changed

+62
-1
lines changed

5 files changed

+62
-1
lines changed
Binary file not shown.

test/inputs/groups/group_v1/__tiledb_group.tdb

Whitespace-only changes.

test/inputs/groups/group_v1/subgroup1/__tiledb_group.tdb

Whitespace-only changes.

test/src/unit-backwards_compat.cc

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,12 +34,15 @@
3434
#include <test/support/tdb_catch.h>
3535
#include "test/support/src/helpers.h"
3636
#include "test/support/src/serialization_wrappers.h"
37+
#include "test/support/src/temporary_local_directory.h"
3738
#include "tiledb/common/common.h"
39+
#include "tiledb/common/stdx_string.h"
3840
#include "tiledb/sm/cpp_api/tiledb"
3941
#include "tiledb/sm/cpp_api/tiledb_experimental"
4042
#include "tiledb/sm/misc/constants.h"
4143

4244
#include <chrono>
45+
#include <filesystem>
4346
#include <iostream>
4447
#include <sstream>
4548
#include <thread>
@@ -52,6 +55,9 @@ namespace {
5255
static const std::string arrays_dir =
5356
std::string(TILEDB_TEST_INPUTS_DIR) + "/arrays";
5457

58+
static const std::string groups_dir =
59+
std::string(TILEDB_TEST_INPUTS_DIR) + "/groups";
60+
5561
template <typename T>
5662
void set_query_coords(
5763
const Domain& domain,
@@ -1428,3 +1434,53 @@ TEST_CASE(
14281434
g, "u64", TILEDB_UINT64, 0x7777777777777777);
14291435
}
14301436
}
1437+
1438+
TEST_CASE(
1439+
"Backwards compatibility: Test v1 groups",
1440+
"[backwards-compat][group][v1]") {
1441+
Context ctx;
1442+
VFS vfs(ctx);
1443+
1444+
// Copy the group to a temporary directory because we will be modifying it.
1445+
tiledb::sm::TemporaryLocalDirectory temp_dir;
1446+
std::filesystem::copy(
1447+
groups_dir + "/group_v1",
1448+
temp_dir.path(),
1449+
std::filesystem::copy_options::recursive);
1450+
1451+
// Read the group
1452+
{
1453+
Group g{ctx, temp_dir.path(), TILEDB_READ};
1454+
1455+
CHECK(g.dump(false) != "");
1456+
CHECK(g.member_count() == 1);
1457+
}
1458+
1459+
// Add a member to the group
1460+
{
1461+
Group g{ctx, temp_dir.path(), TILEDB_WRITE};
1462+
1463+
Group::create(ctx, temp_dir.path() + "/subgroup2");
1464+
1465+
g.add_member("subgroup2", true, "subgroup2");
1466+
1467+
g.close();
1468+
}
1469+
1470+
// Read the group again
1471+
{
1472+
Group g{ctx, temp_dir.path(), TILEDB_READ};
1473+
1474+
CHECK(g.dump(false) != "");
1475+
CHECK(g.member_count() == 2);
1476+
CHECK(g.member(1).name() == "subgroup2");
1477+
}
1478+
1479+
// Read the raw group details files
1480+
auto children = vfs.ls(temp_dir.path() + "/__group");
1481+
CHECK(children.size() == 2);
1482+
std::sort(children.begin(), children.end());
1483+
CHECK(!tiledb::sm::utils::parse::ends_with(children[0], "_1"));
1484+
// This is the file written by this test.
1485+
CHECK(tiledb::sm::utils::parse::ends_with(children[1], "_1"));
1486+
}

tiledb/sm/group/group.cc

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
#include "tiledb/sm/group/group.h"
3434
#include "tiledb/common/common.h"
3535
#include "tiledb/common/logger.h"
36+
#include "tiledb/common/stdx_string.h"
3637
#include "tiledb/sm/enums/datatype.h"
3738
#include "tiledb/sm/enums/encryption_type.h"
3839
#include "tiledb/sm/enums/query_type.h"
@@ -775,8 +776,12 @@ void Group::load_group_details() {
775776

776777
// V1 groups did not have the version appended so only have 4 "_"
777778
// (__<timestamp>_<timestamp>_<uuid>)
779+
// Since 2.19, V1 groups also have the version appended so we have
780+
// to check for that as well
778781
auto part = latest_group_uri.last_path_part();
779-
if (std::count(part.begin(), part.end(), '_') == 4) {
782+
auto underscoreCount = std::count(part.begin(), part.end(), '_');
783+
if (underscoreCount == 4 ||
784+
(underscoreCount == 5 && utils::parse::ends_with(part, "_1"))) {
780785
load_group_from_uri(latest_group_uri);
781786
return;
782787
}

0 commit comments

Comments
 (0)