Add BlankNode support for SERVICE #1504

UNEXENU · 2024-09-19T10:29:04Z

With this commit, QLever supports to add new blank nodes during the evaluation of a query. This function is used to support blank nodes in the result of a SERVICE request. These blank nodes are distinct from all blank nodes in the index, and also from all blank nodes from other SERVICE request, eve if they came from the same server. This behavior is correct accordin to the SPARQL 1.1 federated query standard.

codecov · 2024-09-19T11:00:31Z

Codecov Report

Attention: Patch coverage is 98.50746% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.94%. Comparing base (2b4e6b3) to head (9a01841).

Files with missing lines	Patch %	Lines
src/util/BlankNodeManager.cpp	96.87%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1504      +/-   ##
==========================================
+ Coverage   88.93%   88.94%   +0.01%     
==========================================
  Files         364      366       +2     
  Lines       33127    33194      +67     
  Branches     3715     3722       +7     
==========================================
+ Hits        29462    29526      +64     
- Misses       2433     2439       +6     
+ Partials     1232     1229       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joka921

I have looked at everything but the tests. This already looks nice.
You have forgotten some switch statements that use the Datatype of an ID
(at least in ExportQueryExecutionTrees.cpp. But search for all occurences of the BlankNodeIndex, in every occasion you should also do at least something for NewBlankNodeIndex.

joka921 · 2024-09-19T15:33:47Z

src/engine/QueryExecutionContext.h

@@ -116,6 +118,17 @@ class QueryExecutionContext {
    updateCallback_(nlohmann::ordered_json(runtimeInformation).dump());
  }

+  // Getter for the global `NEXT_NEWBLANKNODEINDEX` counter.
+  [[nodiscard]] static int getNextNewBlankNodeIndex() {


Don't make this function static, even though it can currently be. The staticness should in this case be an implementation detail.

joka921 · 2024-09-19T15:37:51Z

src/engine/QueryExecutionContext.h

+    if (NEXT_NEWBLANKNODEINDEX == std::numeric_limits<uint64_t>::max())
+        [[unlikely]] {
+      throw std::runtime_error(
+          "Counter for blank nodes overflowed. Tell the developers to reset "
+          "it.");
+    }
+    return NEXT_NEWBLANKNODEINDEX++;
+  }


Unfortunately this has a race condition (there is one blank node left, two threads first run the check in line 123,
(which for both is okay), then they both increment by one , upps overflow.
You either have to work with a compare_exchange_weak loop (I am sure that cppreference has an example)
or with a std::mutex.
Also you could here directly also implement the function getMultipleNewBlankNodes that reserves several nodes (e.g. 100) at once for performance reasons.

joka921 · 2024-09-19T15:39:07Z

src/global/Constants.h

@@ -245,3 +246,6 @@ constexpr inline size_t NUM_SORT_THREADS = 4;
 constexpr inline std::string_view EMPH_ON = "\033[1m";
 /// ANSI escape sequence to print "normal" text again in the console.
 constexpr inline std::string_view EMPH_OFF = "\033[22m";
+
+// Counter for internal Blank Node Ids.
+inline std::atomic_uint64_t NEXT_NEWBLANKNODEINDEX{0};


Better call them (here and everywhere local blank nodes (not new). Then it is consistent with the local vocab, which is a similar concept.

joka921 · 2024-09-19T15:39:37Z

src/global/Constants.h

@@ -245,3 +246,6 @@ constexpr inline size_t NUM_SORT_THREADS = 4;
 constexpr inline std::string_view EMPH_ON = "\033[1m";
 /// ANSI escape sequence to print "normal" text again in the console.
 constexpr inline std::string_view EMPH_OFF = "\033[22m";
+
+// Counter for internal Blank Node Ids.


Suggested change

// Counter for internal Blank Node Ids.

// Counter for internal Blank Node Ids that are not part of the index, but were locally added (by SERVICE clauses or expressions).

joka921 · 2024-09-19T15:48:05Z

src/engine/Service.cpp

+    if (!blankNodeMap.contains(value)) {
+      // create a new blankNodeIndex
+      blankNodeMap[value] =
+          Id::makeFromNewBlankNodeIndex(NewBlankNodeIndex::make(
+              getExecutionContext()->getNextNewBlankNodeIndex()));
+    }


This has to compute the hash of value twice or three times in all cases.
Use try_emplace. You would have to first insert a dummy Id as the value, but that should not be a problem here.
(see cppreference, ask me if there's trouble).

src/engine/Service.cpp

joka921

Several small, and 1.5 big requests:
In particular,

Store the highest BlankNodeIndex s.t. you don't need the LocalBlankNodeIndex
(necessary if you implement 1.) Don't use a global variable for the blank node manager, but always access it via the corresponding Index.

joka921 · 2024-10-08T14:47:57Z

src/engine/ExportQueryExecutionTrees.cpp

@@ -217,6 +217,9 @@ ExportQueryExecutionTrees::idToStringAndTypeForEncodedValue(Id id) {
    case BlankNodeIndex:
      return std::pair{absl::StrCat("_:bn", id.getBlankNodeIndex().get()),
                       nullptr};
+    case LocalBlankNodeIndex:
+      return std::pair{absl::StrCat("_:bn", id.getLocalBlankNodeIndex().get()),
+                       nullptr};


I think we should the index of the highest blank node in the index during index building
(It is created in the VocabularyMerger, should be returned from there and can be added during the index building via IndexImpl::writeConfiguration (to the metaData.json file). It should be read from there during the reading of the index builder from file. The LocalBlankNodeManager can then be a member of the Index class, and we don't need the additional datatype LocalBlankNodeIndex. (Datatypes are valuable).

joka921 · 2024-10-08T14:51:05Z

src/engine/Service.cpp

-        "For now, consider filtering them out using the ISBLANK function "
-        "or "
-        "converting them via the STR function.");
+    auto optEmplace = blankNodeMap.try_emplace(value, Id());


Suggested change

auto optEmplace = blankNodeMap.try_emplace(value, Id());

auto [it, wasNew] = blankNodeMap.try_emplace(value, Id());

joka921 · 2024-10-08T14:54:10Z

src/util/BlankNodeManager.h

+ public:
+  static const uint blockSize_ = 1000;
+  static constexpr uint64_t totalAvailableBlocks_ =
+      (ValueId::maxIndex + 1) / blockSize_;


Is the +1 correct?

and is it necessary? one block less for better readability shouldn't be much of an issue.

joka921 · 2024-10-08T14:56:23Z

src/util/BlankNodeManager.h

+namespace ad_utility {
+/*
+ * Manager class for LocalBlankNodeIndex.
+ */


Please elaborate a little bit, e.g. what are local blank node indices, what does this class do/manage, etc.

joka921 · 2024-10-08T14:56:37Z

src/util/BlankNodeManager.h

+ */
+class BlankNodeManager {
+ public:
+  static const uint blockSize_ = 1000;


can also be constexpr.

joka921 · 2024-10-08T14:58:58Z

src/util/BlankNodeManager.h

+    uint64_t nextIdx_;
+  };
+
+  // Manages the LocalBlankNodes used within a LocalVocab.


Please also document the lifetime implications (how long does what have to be kept alive to savely work with the blank node indices.

joka921 · 2024-10-08T15:00:50Z

src/util/BlankNodeManager.h

+
+}  // namespace ad_utility
+
+inline ad_utility::BlankNodeManager globalBlankNodeManager;


With my proposed change from above, you have to get rid of the global blank node manager, so to create a blank node manager, you need access to something. (e.g. a queryExecutionContext).

joka921 · 2024-10-08T15:02:45Z

src/util/BlankNodeManager.cpp

+
+  // Lock this part, as it might be accessed concurrently by
+  // `LocalBlankNodeManager` instances.
+  mtx_.lock();


use a std::lock_guard object instead of manually (un)-locking the object.

joka921 · 2024-10-08T15:04:48Z

src/util/BlankNodeManager.cpp

+// _____________________________________________________________________________
+void BlankNodeManager::freeBlock(uint64_t blockIndex) {
+  usedBlocksSet_.erase(blockIndex);
+}


Erasing the blocks must also lock a mutex.
Best use the ad_utility::Synchronized class for access to the usedBlockSet (see the header + tests for documentation).

joka921 · 2024-10-08T15:08:24Z

test/ServiceTest.cpp

+  // BlankNode exists already, known Id will be used.
+  Id a2 =
+      bTTC({{"type", "bnode"}, {"value", "A"}}).toValueIdIfNotString().value();
+  EXPECT_EQ(a, a2);


Also check the datatype of the blank node index.

…service-blank-nodes

joka921

A thorough review on everything but the tests.
There's not that much to do, this looks very nice.

src/engine/LocalVocab.cpp

src/index/Index.cpp

joka921 · 2024-10-15T09:15:11Z

src/index/VocabularyMerger.h

@@ -62,7 +62,7 @@ struct VocabularyMetaData {
    Id begin() const { return begin_; }
    Id end() const { return end_; }

-    // Return true iff the `id` belongs to this range.
+    // Return true if the `id` belongs to this range.


Suggested change

// Return true if the `id` belongs to this range.

// Return true iff the `id` belongs to this range.

That is not really a typo.
In mathematics and therefore also in computer since you write iff with two ffs for if and only if to be more precise.

joka921 · 2024-10-15T09:18:56Z

src/util/BlankNodeManager.cpp

+  // The Random-Generation Algorithm's performance is reduced once the number of
+  // used blocks exceeds a limit.
+  AD_CORRECTNESS_CHECK(usedBlocksSet_.rlock()->size() <
+                       totalAvailableBlocks_ / 256);


Add a message to this check (the second argument to the CORRECTNESS_CHECK can be a string or a lambda that returns a string., s.t. we know, what has gone wrong here.

joka921 · 2024-10-15T09:23:58Z

src/util/BlankNodeManager.cpp

+  for (auto block : blocks_) {
+    blankNodeManager_->freeBlock(block.blockIdx_);
+  }


This takes one lock + unlock per block, which is a waste of resources.
You can either create a templated function freeBlocks s.t. you here can write manager_->freeBlocks(blocks_ | std::views::transform(...)).
or you make the LocalBlankNodeManager a friend of the BlankNodeManager and handle the locking here yourself.

joka921 · 2024-10-15T09:25:44Z

src/util/BlankNodeManager.cpp

+      blocks_.back().nextIdx_ ==
+          (blankNodeManager_->minIndex_ + blocks_.back().blockIdx_ + 1) *
+              blockSize_) {


I think you can explicitly store the lastIndex or the endIndex in the Block , that way you don't have to fiddle with the internals here.

test/LocalVocabTest.cpp

joka921

Also,
This will be an index-breaking change (because you have to know the number of blank nodes), so you have to change something in the IndexFormatVersion.h

joka921

This is great, almost finished, only minor suggestions remaining.
Make sure to also fix that one trivial sonarcloud thing (public member var in indexImpl, can definitely be private.

joka921 · 2024-10-18T05:52:42Z

src/engine/LocalVocab.cpp

  // Initialize the `localBlankNodeManager_` if it doesn't exist yet.
  if (!localBlankNodeManager_) [[unlikely]] {
    localBlankNodeManager_ =
-        std::make_unique<ad_utility::BlankNodeManager::LocalBlankNodeManager>(
-            blankNodeManager);
+        ad_utility::BlankNodeManager::LocalBlankNodeManager(blankNodeManager);


I think for optional you can write
localBlankNodeManager_.emplace(blankNodeManager) , emplace also works for explicit constructors.

joka921 · 2024-10-18T05:55:39Z

src/util/BlankNodeManager.cpp

+      usedBlocksSet_.rlock()->size() < totalAvailableBlocks_ / 256,
+      absl::StrCat("Critical high number of blank node blocks in use: ",
+                   usedBlocksSet_.rlock()->size(), " blocks"));


this should in priniple (because of rlock) be deadlock-free, but stil pecompute
auto numBlocks = ... and then use that variable in the check to reduce the code duplication.

joka921 · 2024-10-18T05:57:39Z

src/util/BlankNodeManager.cpp

+  auto usedBlocksSetPtr = usedBlocksSet_.wlock();
+  while (usedBlocksSetPtr->contains(newBlockIndex)) {
+    newBlockIndex = randBlockIndex_();
  }
+  usedBlocksSetPtr->insert(newBlockIndex);


The two lines withrandBlockIndex() can be refactored, what about

while(true) { auto idx = randIndx(); if not contained(idx) { insert + return; }

joka921 · 2024-10-18T06:01:14Z

src/util/BlankNodeManager.h

+    LocalBlankNodeManager& operator=(const LocalBlankNodeManager&) = delete;
+
+    LocalBlankNodeManager(LocalBlankNodeManager&&) = default;
+    LocalBlankNodeManager& operator=(LocalBlankNodeManager&&) = default;


That is a good point of codecov:
Add a unit test, that moving of the local blank node managers does the freeing of the blocks still only once and at the right place. That makes us robust on ou assuptions on the moving of std::vector

joka921 · 2024-10-18T06:01:57Z

src/util/CMakeLists.txt

@@ -2,4 +2,4 @@ add_subdirectory(ConfigManager)
 add_subdirectory(MemorySize)
 add_subdirectory(http)
 add_library(util GeoSparqlHelpers.cpp antlr/ANTLRErrorHandling.cpp ParseException.cpp Conversions.cpp Date.cpp DateYearDuration.cpp Duration.cpp antlr/GenerateAntlrExceptionMetadata.cpp CancellationHandle.cpp StringUtils.cpp LazyJsonParser.cpp BlankNodeManager.cpp)
-qlever_target_link_libraries(util re2::re2)
+qlever_target_link_libraries(util re2::re2 s2)


You are right... thanks.

joka921 · 2024-10-18T06:04:58Z

test/IndexTest.cpp

+
+  const Index& index2 = getQec("")->getIndex();
+  EXPECT_NO_THROW(index2.getBlankNodeManager());
+}


Also add a test for the correct reading and writing of the minimal local blank node index
just use getQec(...) with a kg that has some blank nodes, and then access the blank node manager of the returned index to check that the number of blank nodes was propagated correctly.

joka921

Thank you very much

sparql-conformance · 2024-10-18T17:25:49Z

Conformance check passed ✅

No test result changes.

Details: https://qlever.cs.uni-freiburg.de/sparql-conformance-ui?cur=f572c7acea16d6b9763f652185b4d4f73e6e7092&prev=e53d783a4dc07500296b2e891b29c395b9433ecb

sonarqubecloud · 2024-10-18T18:59:53Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

#1617) This resolves a bug that was introduced in #1504, where the `LocalBlankNodeManager` of a `LocalVocab` was not accounted for in the merge functions of the `LocalVocab`.

add BlankNode support for the service-clause

026d3fa

joka921 requested changes Sep 19, 2024

View reviewed changes

UNEXENU added 5 commits September 20, 2024 13:24

various improvements

961f8e1

added BlankNodeManager

521d4ca

Merge branch 'master' into service-blank-nodes

d54b505

cleanup tests

a43ae9c

Merge branch 'master' into service-blank-nodes

7b52104

joka921 requested changes Oct 8, 2024

View reviewed changes

UNEXENU added 5 commits October 10, 2024 11:04

Removed LocalBlankNodeIndex-type

54c15a5

add BlankNodeManager to Index

735bc5f

Merge remote-tracking branch 'upstream/master' into service-blank-nodes

153a951

Merge branch 'service-blank-nodes' of github.com:unexenu/qlever into …

84384cd

…service-blank-nodes

remove deprecated function call

8ba39a5

joka921 requested changes Oct 15, 2024

View reviewed changes

UNEXENU added 4 commits October 16, 2024 17:04

Merge remote-tracking branch 'upstream/master' into service-blank-nodes

6263eda

various improvements

26067c3

fix tests

c354fa0

Merge remote-tracking branch 'upstream/master' into service-blank-nodes

b2fa41d

joka921 requested changes Oct 18, 2024

View reviewed changes

UNEXENU and others added 3 commits October 18, 2024 09:55

Merge remote-tracking branch 'upstream/master' into service-blank-nodes

5e43c5e

various improvements

9a01841

Update IndexFormatVersion.h

f572c7a

joka921 approved these changes Oct 18, 2024

View reviewed changes

joka921 merged commit ef057ac into ad-freiburg:master Oct 18, 2024
18 checks passed

joka921 mentioned this pull request Oct 18, 2024

Failed to parse the Service result as JSON #1427

Open

UNEXENU mentioned this pull request Nov 14, 2024

Also merge the LocalBlankNodeManagers when a LocalVocab is merged. #1617

Merged

	// Counter for internal Blank Node Ids.
	// Counter for internal Blank Node Ids that are not part of the index, but were locally added (by SERVICE clauses or expressions).

	auto optEmplace = blankNodeMap.try_emplace(value, Id());
	auto [it, wasNew] = blankNodeMap.try_emplace(value, Id());


		} // namespace ad_utility

		inline ad_utility::BlankNodeManager globalBlankNodeManager;

	// Return true if the `id` belongs to this range.
	// Return true iff the `id` belongs to this range.

Add BlankNode support for SERVICE #1504

Add BlankNode support for SERVICE #1504

Conversation

UNEXENU commented Sep 19, 2024 • edited by joka921 Loading

codecov bot commented Sep 19, 2024 • edited Loading

Codecov Report

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

sparql-conformance bot commented Oct 18, 2024

Conformance check passed ✅

sonarqubecloud bot commented Oct 18, 2024

Quality Gate passed

UNEXENU commented Sep 19, 2024 •

edited by joka921

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading