Add the possibility to explicitly allocate blank node blocks #2574

joka921 · 2025-12-04T15:32:36Z

This will be used for reading the updateTriples and serialized cached results.

Signed-off-by: Johannes Kalmbach <[email protected]>

…out providing any unit tests yet. Signed-off-by: Johannes Kalmbach <[email protected]>

codecov · 2025-12-05T11:07:06Z

Codecov Report

❌ Patch coverage is 94.20290% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.20%. Comparing base (6c4c294) to head (312094a).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
src/util/BlankNodeManager.cpp	93.61%	1 Missing and 5 partials ⚠️
src/engine/LocalVocab.h	0.00%	1 Missing ⚠️
src/util/Synchronized.h	95.23%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2574      +/-   ##
==========================================
+ Coverage   91.14%   91.20%   +0.06%     
==========================================
  Files         469      473       +4     
  Lines       40181    40338     +157     
  Branches     5376     5390      +14     
==========================================
+ Hits        36622    36791     +169     
+ Misses       2021     2009      -12     
  Partials     1538     1538

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

RobinTF

Started a first review. I still don't quite understand all the quirks.

src/util/BlankNodeManager.h

src/engine/LocalVocab.h

src/util/BlankNodeManager.h

RobinTF · 2025-12-05T10:53:36Z

src/util/BlankNodeManager.h

+    // is assigned a UUID. This map keeps track of the currently active sets,
+    // but does not participate in their (shared) ownership, hence the
+    // `weak_ptr`.
+    std::unordered_map<boost::uuids::uuid, std::weak_ptr<Blocks>, UuidHash>


Why not a adu::HashMap?

src/util/BlankNodeManager.cpp

src/util/BlankNodeManager.h

Signed-off-by: Johannes Kalmbach <[email protected]>

RobinTF

We're getting there. Didn't have an in depth look at the individual tests in test/BlankNodeManagerTest.cpp, but they all look reasonable.

RobinTF · 2025-12-05T16:36:33Z

src/engine/LocalVocab.cpp

+
+// _____________________________________________________________________________
+auto LocalVocab::getOwnedLocalBlankNodeBlocks() const
+    -> std::vector<LocalBlankNodeManager::OwnedBlocksEntry> {


Any particular reason why the auto -> syntax is used instead of a directly specified return type?

RobinTF · 2025-12-05T16:40:04Z

src/util/BlankNodeManager.cpp

+    // We unfortunately cannot make this an `AD_CORRECTNESS_CHECK`, because then
+    // we might have a deadlock wrt the `Blocks` destructor.


But why? Doesn't AD_CORRECTNESS_CHECK also just throw an exception?

src/util/BlankNodeManager.cpp

RobinTF · 2025-12-05T16:49:38Z

src/util/BlankNodeManager.cpp

+  auto ptr = isNew ? std::shared_ptr<Blocks>() : it->second.lock();
+  if (isNew || ptr == nullptr) {


These two lines are confusing. Would it be incorrect to just do

Suggested change

auto ptr = isNew ? std::shared_ptr<Blocks>() : it->second.lock();

if (isNew || ptr == nullptr) {

auto ptr = it->second.lock();

if (ptr == nullptr)

in particular, I find it really hard to reason about this since the value of ptr also depends on isNew

src/util/BlankNodeManager.h

RobinTF · 2025-12-05T16:54:43Z

test/BlankNodeManagerTest.cpp

+    return lbnm;
+  }
+
+  // Helper to perform a round-trip serialization/deserialization


Please add full stops to all of these.

RobinTF · 2025-12-05T16:56:29Z

test/BlankNodeManagerTest.cpp

+  // Helper to verify all IDs are contained in the LocalBlankNodeManager
+  static void verifyIdsContained(
+      const BlankNodeManager::LocalBlankNodeManager& lbnm,
+      const std::vector<uint64_t>& ids) {


Consider adding a source location + location trace here. Same for all the other functions containing EXPECT/ASSERT.

RobinTF · 2025-12-05T16:57:46Z

test/BlankNodeManagerTest.cpp

+  // Verify new IDs can still be allocated and don't conflict
+  auto newId = lbnm2->getId();
+  EXPECT_TRUE(lbnm2->containsBlankNodeIndex(newId));
+  // The new ID should be in a new block (primary blocks now has 1 block)


Same here with full stops.

RobinTF · 2025-12-05T16:58:05Z

test/BlankNodeManagerTest.cpp

+  auto originalIds = allocateIdsAcrossBlocks(*lbnm, 3);
+  ASSERT_EQ(getPrimaryBlocks(*lbnm).size(), 3);
+
+  // Serialize


Signed-off-by: Johannes Kalmbach <[email protected]>

RobinTF

I think your automatic changes went a bit too far.

test/BlankNodeManagerTest.cpp

RobinTF · 2025-12-05T20:42:34Z

test/BlankNodeManagerTest.cpp

-          BlankNodeManager::LocalBlankNodeManager::OwnedBlocksEntry>& entries) {
+          BlankNodeManager::LocalBlankNodeManager::OwnedBlocksEntry>& entries,
+      ad_utility::source_location loc = AD_CURRENT_SOURCE_LOC()) {
+    auto t = generateLocationTrace(loc);


RobinTF · 2025-12-05T20:42:47Z

test/BlankNodeManagerTest.cpp

+      BlankNodeManager* bnm,
+      const BlankNodeManager::LocalBlankNodeManager& source,
+      ad_utility::source_location loc = AD_CURRENT_SOURCE_LOC()) {
+    auto t = generateLocationTrace(loc);


test/BlankNodeManagerTest.cpp

Signed-off-by: Johannes Kalmbach <[email protected]>

RobinTF

Please remove the remaining comment (and consider the reference) then this is good to go 👍🏼

RobinTF · 2025-12-08T11:10:08Z

test/BlankNodeManagerTest.cpp

+    //[[maybe_unused]] uint64_t id = lbnm.getId();
+    // EXPECT_EQ(bnm.state_.rlock()->usedBlocksSet_.size(), 1);


Please remove this comment.

RobinTF · 2025-12-08T11:14:05Z

src/util/Synchronized.h

+  ptr_type s() const {
+    AD_CORRECTNESS_CHECK(s_ != nullptr);
+    return s_;
+  }


Did you consider at one point to return a reference here?

Signed-off-by: Johannes Kalmbach <[email protected]>

RobinTF

🎉

sparql-conformance · 2025-12-08T15:02:32Z

Overview

Number of Tests	Passed ✅	Intended ✅	Failed ❌	Not tested
525	379	67	79	0

Conformance check passed ✅

No test result changes.

Details: https://qlever.dev/sparql-conformance-ui?cur=312094a43c80757d808d229a0aaec0e68fb8c3cb&prev=50d08bc03a4c1fe7495d4209a3542c90ce36b997

sonarqubecloud · 2025-12-08T16:43:13Z

Quality Gate failed

Failed conditions
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

joka921 added 3 commits December 4, 2025 16:31

Add the possibility to explicitly allocate blank node blocks.

2a522f9

Signed-off-by: Johannes Kalmbach <[email protected]>

This was quite a ride...

eb14a6b

Signed-off-by: Johannes Kalmbach <[email protected]>

A thorough refactoring, that I boldly claim to be gbug free, but with…

715d917

…out providing any unit tests yet. Signed-off-by: Johannes Kalmbach <[email protected]>

joka921 marked this pull request as ready for review December 5, 2025 10:07

joka921 requested a review from RobinTF December 5, 2025 10:07

Merge branch 'master' into serialize-named-cached-queries-2

9e98dec

RobinTF requested changes Dec 5, 2025

View reviewed changes

joka921 added 5 commits December 5, 2025 13:31

Some changes from a review.

46b0fed

Signed-off-by: Johannes Kalmbach <[email protected]>

Also add Tests for the local vocab.

ad4205e

Signed-off-by: Johannes Kalmbach <[email protected]>

Fix the tests.

d6dc143

Signed-off-by: Johannes Kalmbach <[email protected]>

Fix the compilation

4376c26

Signed-off-by: Johannes Kalmbach <[email protected]>

Another round of updates...

d5cdd9e

Signed-off-by: Johannes Kalmbach <[email protected]>

joka921 requested a review from RobinTF December 5, 2025 16:35

RobinTF requested changes Dec 5, 2025

View reviewed changes

joka921 added 2 commits December 5, 2025 19:22

Another round of reviews.

fea560f

Signed-off-by: Johannes Kalmbach <[email protected]>

Final updates from Robin'sreview (hopefully)

66af555

Signed-off-by: Johannes Kalmbach <[email protected]>

RobinTF requested changes Dec 5, 2025

View reviewed changes

A round of further fixes and updates.

d25bda9

Signed-off-by: Johannes Kalmbach <[email protected]>

joka921 requested a review from RobinTF December 8, 2025 09:34

RobinTF requested changes Dec 8, 2025

View reviewed changes

The last suggestions made by Robin!

312094a

Signed-off-by: Johannes Kalmbach <[email protected]>

RobinTF approved these changes Dec 8, 2025

View reviewed changes

		// We unfortunately cannot make this an `AD_CORRECTNESS_CHECK`, because then
		// we might have a deadlock wrt the `Blocks` destructor.

		auto ptr = isNew ? std::shared_ptr<Blocks>() : it->second.lock();
		if (isNew \|\| ptr == nullptr) {

		//[[maybe_unused]] uint64_t id = lbnm.getId();
		// EXPECT_EQ(bnm.state_.rlock()->usedBlocksSet_.size(), 1);

Add the possibility to explicitly allocate blank node blocks #2574

Are you sure you want to change the base?

Add the possibility to explicitly allocate blank node blocks #2574

Conversation

joka921 commented Dec 4, 2025

Uh oh!

codecov bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

RobinTF left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RobinTF left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RobinTF left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RobinTF left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RobinTF left a comment

Choose a reason for hiding this comment

Uh oh!

sparql-conformance bot commented Dec 8, 2025

Overview

Conformance check passed ✅

Uh oh!

sonarqubecloud bot commented Dec 8, 2025

Quality Gate failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

codecov bot commented Dec 5, 2025 •

edited

Loading