code review

ad-freiburg · Feb 10, 2025 · dffd916 · dffd916
1 parent b99fdc0
commit dffd916
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 13 deletions.
diff --git a/src/engine/ExecuteUpdate.cpp b/src/engine/ExecuteUpdate.cpp
@@ -176,21 +176,29 @@ ExecuteUpdate::computeGraphUpdateQuads(
       cancellationHandle->throwIfCancelled();
     }
   }
-  auto sortAndRemoveDuplicates = [](auto& vec) {
-    ql::ranges::sort(vec);
-    vec.erase(std::unique(vec.begin(), vec.end()), vec.end());
-  };
   sortAndRemoveDuplicates(toInsert);
   sortAndRemoveDuplicates(toDelete);
-  metadata.inUpdate = DeltaTriplesCount{static_cast<int64_t>(toInsert.size()),
-                                        static_cast<int64_t>(toDelete.size())};
-  std::vector<IdTriple<>> reducedToDelete;
-  ql::ranges::set_difference(std::move(toDelete), toInsert,
-                             std::back_inserter(reducedToDelete));
-  toDelete.swap(reducedToDelete);
+  metadata.inUpdate_ = DeltaTriplesCount{static_cast<int64_t>(toInsert.size()),
+                                         static_cast<int64_t>(toDelete.size())};
+  toDelete = setMinus(toDelete, toInsert);
   metadata.triplePreparationTime_ = timer.msecs();
 
   return {
       IdTriplesAndLocalVocab{std::move(toInsert), std::move(localVocabInsert)},
       IdTriplesAndLocalVocab{std::move(toDelete), std::move(localVocabDelete)}};
 }
+
+void ExecuteUpdate::sortAndRemoveDuplicates(
+    std::vector<IdTriple<>>& container) {
+  ql::ranges::sort(container);
+  container.erase(std::unique(container.begin(), container.end()),
+                  container.end());
+}
+
+std::vector<IdTriple<>> ExecuteUpdate::setMinus(
+    const std::vector<IdTriple<>>& a, const std::vector<IdTriple<>>& b) {
+  std::vector<IdTriple<>> reducedToDelete;
+  ql::ranges::set_difference(std::move(a), b,
+                             std::back_inserter(reducedToDelete));
+  return reducedToDelete;
+}
diff --git a/src/engine/ExecuteUpdate.h b/src/engine/ExecuteUpdate.h
@@ -17,7 +17,7 @@ struct UpdateMetadata {
   Milliseconds triplePreparationTime_ = Zero;
   Milliseconds insertionTime_ = Zero;
   Milliseconds deletionTime_ = Zero;
-  std::optional<DeltaTriplesCount> inUpdate;
+  std::optional<DeltaTriplesCount> inUpdate_;
 };
 
 class ExecuteUpdate {
@@ -72,4 +72,15 @@ class ExecuteUpdate {
                           const CancellationHandle& cancellationHandle,
                           UpdateMetadata& metadata);
   FRIEND_TEST(ExecuteUpdate, computeGraphUpdateQuads);
+
+  // After the operation the vector is sorted and contains no duplicate
+  // elements.
+  static void sortAndRemoveDuplicates(std::vector<IdTriple<>>& container);
+  FRIEND_TEST(ExecuteUpdate, sortAndRemoveDuplicates);
+
+  // For two sorted vectors `A` and `B` return a new vector
+  // that contains the element of `A\B`.
+  static std::vector<IdTriple<>> setMinus(const std::vector<IdTriple<>>& a,
+                                          const std::vector<IdTriple<>>& b);
+  FRIEND_TEST(ExecuteUpdate, setMinus);
 };
diff --git a/src/engine/Server.cpp b/src/engine/Server.cpp
@@ -961,9 +961,9 @@ json Server::createResponseMetadataForUpdate(
   response["delta-triples"]["after"] = nlohmann::json(countAfter);
   response["delta-triples"]["difference"] =
       nlohmann::json(countAfter - countBefore);
-  if (updateMetadata.inUpdate.has_value()) {
+  if (updateMetadata.inUpdate_.has_value()) {
     response["delta-triples"]["operation"] =
-        json(updateMetadata.inUpdate.value());
+        json(updateMetadata.inUpdate_.value());
   }
   response["time"]["planning"] =
       formatTime(runtimeInfoWholeOp.timeQueryPlanning);

diff --git a/src/index/DeltaTriples.cpp b/src/index/DeltaTriples.cpp
@@ -150,6 +150,9 @@ void DeltaTriples::modifyTriplesImpl(CancellationHandle cancellationHandle,
                                      TriplesToHandlesMap& targetMap,
                                      TriplesToHandlesMap& inverseMap) {
   rewriteLocalVocabEntriesAndBlankNodes(triples);
+  AD_EXPENSIVE_CHECK(ql::ranges::is_sorted(triples));
+  AD_EXPENSIVE_CHECK(std::unique(triples.begin(), triples.end()) ==
+                     triples.end());
   std::erase_if(triples, [&targetMap](const IdTriple<0>& triple) {
     return targetMap.contains(triple);
   });

diff --git a/test/ExecuteUpdateTest.cpp b/test/ExecuteUpdateTest.cpp
@@ -372,3 +372,45 @@ TEST(ExecuteUpdate, computeAndAddQuadsForResultRow) {
                      ElementsAreArray({IdTriple{{V(0), V(1), V(1), V(3)}},
                                        IdTriple{{V(0), V(1), V(2), V(3)}}}));
 }
+
+TEST(ExecuteUpdate, sortAndRemoveDuplicates) {
+  auto expect = [](std::vector<IdTriple<>> input,
+                   const std::vector<IdTriple<>>& expected,
+                   source_location l = source_location::current()) {
+    auto trace = generateLocationTrace(l);
+    ExecuteUpdate::sortAndRemoveDuplicates(input);
+    EXPECT_THAT(input, testing::ElementsAreArray(expected));
+  };
+  auto IdTriple = [&](uint64_t s, uint64_t p, uint64_t o, uint64_t g = 0) {
+    return ::IdTriple({V(s), V(p), V(o), V(g)});
+  };
+  expect({}, {});
+  expect({IdTriple(1, 1, 1)}, {IdTriple(1, 1, 1)});
+  expect({IdTriple(1, 1, 1), IdTriple(2, 2, 2)},
+         {IdTriple(1, 1, 1), IdTriple(2, 2, 2)});
+  expect({IdTriple(2, 2, 2), IdTriple(1, 1, 1)},
+         {IdTriple(1, 1, 1), IdTriple(2, 2, 2)});
+  expect({IdTriple(1, 1, 1), IdTriple(1, 1, 1)}, {IdTriple(1, 1, 1)});
+  expect({IdTriple(2, 2, 2), IdTriple(3, 3, 3), IdTriple(3, 3, 3),
+          IdTriple(2, 2, 2), IdTriple(1, 1, 1)},
+         {IdTriple(1, 1, 1), IdTriple(2, 2, 2), IdTriple(3, 3, 3)});
+}
+TEST(ExecuteUpdate, setMinus) {
+  auto expect = [](std::vector<IdTriple<>> a, std::vector<IdTriple<>> b,
+                   const std::vector<IdTriple<>>& expected,
+                   source_location l = source_location::current()) {
+    auto trace = generateLocationTrace(l);
+    EXPECT_THAT(ExecuteUpdate::setMinus(a, b),
+                testing::ElementsAreArray(expected));
+  };
+  auto IdTriple = [&](uint64_t s, uint64_t p, uint64_t o, uint64_t g = 0) {
+    return ::IdTriple({V(s), V(p), V(o), V(g)});
+  };
+  expect({}, {}, {});
+  expect({IdTriple(1, 2, 3), IdTriple(4, 5, 6)}, {},
+         {IdTriple(1, 2, 3), IdTriple(4, 5, 6)});
+  expect({IdTriple(1, 2, 3), IdTriple(4, 5, 6), IdTriple(7, 8, 9)},
+         {IdTriple(4, 5, 6), IdTriple(7, 8, 9)}, {IdTriple(1, 2, 3)});
+  expect({IdTriple(1, 2, 3)},
+         {IdTriple(1, 2, 3), IdTriple(4, 5, 6), IdTriple(7, 8, 9)}, {});
+}