Skip to content

Commit

Permalink
Refactor deletion handling and update test assertions
Browse files Browse the repository at this point in the history 
Refactor CRDT deletion handling to use an empty string ("") instead of "__deleted__" as the marker for deleted columns. Update logic for record fields to check emptiness rather than a specific marker. Adjust all related tests to align with the new deletion marker and improve assertion descriptions for clarity.
  • Loading branch information
@sinkingsugar
sinkingsugar committed Nov 16, 2024
1 parent 2cd5f08 commit 139ae4a
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 32 deletions.
25 changes: 11 additions & 14 deletions crdt.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -472,13 +472,15 @@ class CRDT : public std::enable_shared_from_this<CRDT<K, V, MergeRuleType, Chang
if (clock_info.local_db_version > last_db_version && !excluding.contains(clock_info.node_id)) {
std::optional<V> value = std::nullopt;
std::optional<CrdtString> name = std::nullopt;
if (col_name != "__deleted__") {

if (!record.fields.empty()) { // If record has fields, it's not deleted
auto field_it = record.fields.find(col_name);
if (field_it != record.fields.end()) {
value = field_it->second;
}
name = col_name;
}

changes.emplace_back(Change<K, V>(record_id, std::move(name), std::move(value), clock_info.col_version,
clock_info.db_version, clock_info.node_id, clock_info.local_db_version));
}
Expand Down Expand Up @@ -510,7 +512,7 @@ class CRDT : public std::enable_shared_from_this<CRDT<K, V, MergeRuleType, Chang
/// Complexity: O(c), where c is the number of changes to merge
template <bool ReturnAcceptedChanges = false>
std::conditional_t<ReturnAcceptedChanges, CrdtVector<Change<K, V>>, void> merge_changes(CrdtVector<Change<K, V>> &&changes,
bool ignore_parent = false) {
bool ignore_parent = false) {
CrdtVector<Change<K, V>> accepted_changes;

if (changes.empty()) {
Expand Down Expand Up @@ -545,7 +547,7 @@ class CRDT : public std::enable_shared_from_this<CRDT<K, V, MergeRuleType, Chang
const Record<V> *record_ptr = get_record_ptr(record_id, ignore_parent);
const ColumnVersion *local_col_info = nullptr;
if (record_ptr != nullptr) {
auto col_it = record_ptr->column_versions.find(col_name ? *col_name : "__deleted__");
auto col_it = record_ptr->column_versions.find(col_name ? *col_name : "");
if (col_it != record_ptr->column_versions.end()) {
local_col_info = &col_it->second;
}
Expand All @@ -558,7 +560,7 @@ class CRDT : public std::enable_shared_from_this<CRDT<K, V, MergeRuleType, Chang
// No local version exists; accept the remote change
should_accept = true;
} else {
Change<K, V> local_change(record_id, col_name ? *col_name : "__deleted__", std::nullopt, local_col_info->col_version,
Change<K, V> local_change(record_id, col_name ? *col_name : "", std::nullopt, local_col_info->col_version,
local_col_info->db_version, local_col_info->node_id, flags);
should_accept = merge_rule_(local_change, change);
}
Expand All @@ -571,7 +573,7 @@ class CRDT : public std::enable_shared_from_this<CRDT<K, V, MergeRuleType, Chang

// Update deletion clock info
CrdtMap<CrdtString, ColumnVersion> deletion_clock;
deletion_clock.emplace("__deleted__",
deletion_clock.emplace("",
ColumnVersion(remote_col_version, remote_db_version, remote_node_id, new_local_db_version));

// Store deletion info in the data map
Expand Down Expand Up @@ -871,12 +873,9 @@ class CRDT : public std::enable_shared_from_this<CRDT<K, V, MergeRuleType, Chang
tombstones_.emplace(record_id);
data_.erase(record_id);

// Insert deletion clock info
// Store empty record with deletion clock info
CrdtMap<CrdtString, ColumnVersion> deletion_clock;
deletion_clock.emplace("__deleted__",
ColumnVersion(remote_col_version, remote_db_version, remote_node_id, remote_local_db_version));

// Store deletion info in the data map
deletion_clock.emplace("", ColumnVersion(remote_col_version, remote_db_version, remote_node_id, remote_local_db_version));
data_.emplace(record_id, Record<V>(CrdtMap<CrdtString, V>(), std::move(deletion_clock)));
} else {
if (!is_record_tombstoned(record_id)) {
Expand Down Expand Up @@ -1098,11 +1097,9 @@ class CRDT : public std::enable_shared_from_this<CRDT<K, V, MergeRuleType, Chang
tombstones_.emplace(record_id);
data_.erase(record_id);

// Insert deletion clock info
// Create empty record with deletion clock info
CrdtMap<CrdtString, ColumnVersion> deletion_clock;
deletion_clock.emplace("__deleted__", ColumnVersion(1, db_version, node_id_, db_version));

// Store deletion info in the data map
deletion_clock.emplace("", ColumnVersion(1, db_version, node_id_, db_version));
data_.emplace(record_id, Record<V>(CrdtMap<CrdtString, V>(), std::move(deletion_clock)));

if (changes) {
Expand Down
36 changes: 18 additions & 18 deletions tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,12 @@ int main() {
// Both nodes should reflect the deletion
assert_true(node1.get_data().at(record_id).fields.empty(), "Delete and Merge: Node1 should have empty fields");
assert_true(node2.get_data().at(record_id).fields.empty(), "Delete and Merge: Node2 should have empty fields");
assert_true(node1.get_data().at(record_id).column_versions.find("__deleted__") !=
assert_true(node1.get_data().at(record_id).column_versions.find("") !=
node1.get_data().at(record_id).column_versions.end(),
"Delete and Merge: Node1 should have '__deleted__' column version");
assert_true(node2.get_data().at(record_id).column_versions.find("__deleted__") !=
"Delete and Merge: Node1 should have deletion column version");
assert_true(node2.get_data().at(record_id).column_versions.find("") !=
node2.get_data().at(record_id).column_versions.end(),
"Delete and Merge: Node2 should have '__deleted__' column version");
"Delete and Merge: Node2 should have deletion column version");
std::cout << "Test 'Delete and Merge' passed." << std::endl;
}

Expand Down Expand Up @@ -146,9 +146,9 @@ int main() {

// Node2 should respect the tombstone
assert_true(node2.get_data().at(record_id).fields.empty(), "Tombstone Handling: Node2 should have empty fields");
assert_true(node2.get_data().at(record_id).column_versions.find("__deleted__") !=
assert_true(node2.get_data().at(record_id).column_versions.find("") !=
node2.get_data().at(record_id).column_versions.end(),
"Tombstone Handling: Node2 should have '__deleted__' column version");
"Tombstone Handling: Node2 should have deletion column version");
std::cout << "Test 'Tombstone Handling' passed." << std::endl;
}

Expand Down Expand Up @@ -299,12 +299,12 @@ int main() {
// The deletion should prevail
assert_true(node1.get_data().at(record_id).fields.empty(), "Inserting After Deletion: Node1 should have empty fields");
assert_true(node2.get_data().at(record_id).fields.empty(), "Inserting After Deletion: Node2 should have empty fields");
assert_true(node1.get_data().at(record_id).column_versions.find("__deleted__") !=
assert_true(node1.get_data().at(record_id).column_versions.find("") !=
node1.get_data().at(record_id).column_versions.end(),
"Inserting After Deletion: Node1 should have '__deleted__' column version");
assert_true(node2.get_data().at(record_id).column_versions.find("__deleted__") !=
"Inserting After Deletion: Node1 should have deletion column version");
assert_true(node2.get_data().at(record_id).column_versions.find("") !=
node2.get_data().at(record_id).column_versions.end(),
"Inserting After Deletion: Node2 should have '__deleted__' column version");
"Inserting After Deletion: Node2 should have deletion column version");
std::cout << "Test 'Inserting After Deletion' passed." << std::endl;
}

Expand Down Expand Up @@ -797,9 +797,9 @@ int main() {
// Child should now have the record tombstoned
assert_true(child_crdt.get_data_combined().at(record_id).fields.empty(),
"Tombstone Propagation: Child should have empty fields after deletion");
assert_true(child_crdt.get_data_combined().at(record_id).column_versions.find("__deleted__") !=
assert_true(child_crdt.get_data_combined().at(record_id).column_versions.find("") !=
child_crdt.get_data().at(record_id).column_versions.end(),
"Tombstone Propagation: Child should have '__deleted__' column version");
"Tombstone Propagation: Child should have deletion column version");

std::cout << "Test 'Tombstone Propagation from Parent to Child' passed." << std::endl;
}
Expand Down Expand Up @@ -952,9 +952,9 @@ int main() {
// Child should have the record tombstoned
assert_true(child_crdt.get_data().at(record_id).fields.empty(),
"Child Deletion: Child should have empty fields after deletion");
assert_true(child_crdt.get_data().at(record_id).column_versions.find("__deleted__") !=
assert_true(child_crdt.get_data().at(record_id).column_versions.find("") !=
child_crdt.get_data().at(record_id).column_versions.end(),
"Child Deletion: Child should have '__deleted__' column version");
"Child Deletion: Child should have deletion column version");

std::cout << "Test 'Child Deletion Does Not Affect Parent' passed." << std::endl;
}
Expand Down Expand Up @@ -1026,16 +1026,16 @@ int main() {
// Parent should still have the record tombstoned without the new field
assert_true(parent_crdt.get_data().at(record_id).fields.empty(),
"Parent Deletion: Parent should still have empty fields after child insertion attempt");
assert_true(parent_crdt.get_data().at(record_id).column_versions.find("__deleted__") !=
assert_true(parent_crdt.get_data().at(record_id).column_versions.find("") !=
parent_crdt.get_data().at(record_id).column_versions.end(),
"Parent Deletion: Parent should have '__deleted__' column version");
"Parent Deletion: Parent should have deletion column version");

// Child should also respect the tombstone
assert_true(child_crdt.get_data().at(record_id).fields.empty(),
"Parent Deletion: Child should have empty fields after parent's deletion");
assert_true(child_crdt.get_data().at(record_id).column_versions.find("__deleted__") !=
assert_true(child_crdt.get_data().at(record_id).column_versions.find("") !=
child_crdt.get_data().at(record_id).column_versions.end(),
"Parent Deletion: Child should have '__deleted__' column version");
"Parent Deletion: Child should have deletion column version");

std::cout << "Test 'Parent Deletion Prevents Child Insertions' passed." << std::endl;
}
Expand Down

0 comments on commit 139ae4a

Please sign in to comment.