From dffd916eb9aa5f6ae567b85111af82348f0e5e37 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Mon, 10 Feb 2025 18:14:15 +0100 Subject: [PATCH] code review --- src/engine/ExecuteUpdate.cpp | 28 +++++++++++++++--------- src/engine/ExecuteUpdate.h | 13 ++++++++++- src/engine/Server.cpp | 4 ++-- src/index/DeltaTriples.cpp | 3 +++ test/ExecuteUpdateTest.cpp | 42 ++++++++++++++++++++++++++++++++++++ 5 files changed, 77 insertions(+), 13 deletions(-) diff --git a/src/engine/ExecuteUpdate.cpp b/src/engine/ExecuteUpdate.cpp index f44f034352..7f54ea682d 100644 --- 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(toInsert.size()), - static_cast(toDelete.size())}; - std::vector> reducedToDelete; - ql::ranges::set_difference(std::move(toDelete), toInsert, - std::back_inserter(reducedToDelete)); - toDelete.swap(reducedToDelete); + metadata.inUpdate_ = DeltaTriplesCount{static_cast(toInsert.size()), + static_cast(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>& container) { + ql::ranges::sort(container); + container.erase(std::unique(container.begin(), container.end()), + container.end()); +} + +std::vector> ExecuteUpdate::setMinus( + const std::vector>& a, const std::vector>& b) { + std::vector> 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 index a0d5c66312..e6345ff099 100644 --- 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 inUpdate; + std::optional 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>& 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> setMinus(const std::vector>& a, + const std::vector>& b); + FRIEND_TEST(ExecuteUpdate, setMinus); }; diff --git a/src/engine/Server.cpp b/src/engine/Server.cpp index 0ffd890a4a..08494b4549 100644 --- 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 index f72fda0134..c3ecad2bde 100644 --- 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 index 5907e9c833..a1da7bcd62 100644 --- 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> input, + const std::vector>& 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> a, std::vector> b, + const std::vector>& 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)}, {}); +}