Code for locating triples in an existing index #1000

hannahbast · 2023-06-09T14:50:04Z

This is the first part of a series of PRs split of from the large proof-of-concept PR #916, which realizes SPARQL 1.1 Update

codecov · 2023-06-09T15:26:17Z

Codecov Report

Attention: Patch coverage is 87.86611% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 74.21%. Comparing base (f9c9ff4) to head (258231d).
Report is 25 commits behind head on master.

❗ Current head 258231d differs from pull request most recent head 59aae8e. Consider uploading reports for the commit 59aae8e to get more accurate results

Files	Patch %	Lines
src/index/LocatedTriples.cpp	88.63%	2 Missing and 23 partials ⚠️
src/index/LocatedTriples.h	76.47%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #1000       +/-   ##
===========================================
- Coverage   87.63%   74.21%   -13.43%     
===========================================
  Files         308      256       -52     
  Lines       28008    24239     -3769     
  Branches     3138     3064       -74     
===========================================
- Hits        24545    17989     -6556     
- Misses       2324     5016     +2692     
- Partials     1139     1234       +95

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

joka921

I have not yet looked at the tests yet,
But I already have some comments about this and that.

src/global/IdTriple.h

src/index/IndexMetaData.h

src/index/MetaDataHandler.h

src/index/LocatedTriples.h

src/index/LocatedTriples.cpp

joka921 · 2023-06-09T18:41:13Z

src/index/LocatedTriples.cpp

+    // result.
+    while (locatedTriple != locatedTriples.end() &&
+           locatedTriple->rowIndexInBlock == rowIndex &&
+           locatedTriple->existsInIndex == false) {


Is the semantics of existsInIndex always existingTripleThatShallBeDeleted? If so, maybe the name can be more explicit.

At first, I indeed used names in this class like "to be inserted" or "to be deleted". However, I then realized that the semantics of the LocatedTriple class is really independent from what you later do with it. So I prefer names that are close to what the class actually does and not what it is later used for.

joka921 · 2023-06-09T18:44:18Z

src/index/LocatedTriples.cpp

+  auto locatedTripleMatches = [&]() {
+    if constexpr (matchMode == MatchMode::MatchAll) {
+      return true;
+    } else if constexpr (matchMode == MatchMode::MatchId1) {
+      return locatedTriple->id1 == id1;
+    } else if constexpr (matchMode == MatchMode::MatchId1AndId2) {
+      return locatedTriple->id1 == id1 && locatedTriple->id2 == id2;
+    }
+  };


This function can be simpler.
I think the ranges of matching locatedTriples can be expressed as a range of contiguous iterators.
So you can write a function getRelevantTriples(blockIndex, matchMode, id1, id2) that returns the two iterators and get rid of some complexity in the loops below.

Alternatively you can directly specify these two iterators as input to this function and perform this calculation previously

And isn't there an inherent invariant between the block indices and the id1 and id2 and therefore the matching range inside the locatedTriples. Those should be established at the same place (the binary search in the block and the binary search in the locatedTriples.

joka921 · 2023-06-09T18:59:27Z

src/index/LocatedTriples.cpp

+  for (size_t rowIndex = rowIndexInBlockBegin; rowIndex < rowIndexInBlockEnd;
+       ++rowIndex) {
+    // Append triples that are marked for insertion at this `rowIndex` to the
+    // result.
+    while (locatedTriple != locatedTriples.end() &&


This whole algorithm looks to me like the zipper/merge join algorithm with a twist.
Therefore I am a bit confused about the nested loop structure here.
I would also expect a tight loop that inserts all the elements from the block that do not appear at all in the locatedTriples.

I am currently thinking about using my zipperJoinWithUndef abstraction for this,
but you would need to add the indices (which is doable). But I would first write this in the "canonical" zipper style.

joka921 · 2023-06-09T19:05:31Z

test/LocatedTriplesTest.cpp

+#include "index/LocatedTriples.h"
+#include "index/Permutations.h"
+
+// TODO: Why the namespace here? (copied from `test/IndexMetaDataTest.cpp`)


To avoid very strange ODR-violation bugs when another .cpp file also has a member V and the two are linked together. Linking the tests together is necessary for the code coverage to work.

…s from Johannes' review

sonarqubecloud · 2023-06-10T21:39:45Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
23 Code Smells

No Coverage information
0.0% Duplication

joka921

I also had an initial look at the tests.
Please also note the report from Sonarcloud. Many suggestions there are similar to my suggestions.

joka921 · 2023-06-15T12:50:16Z

src/index/LocatedTriples.cpp

  std::copy(lts.begin(), lts.end(),
-            std::ostream_iterator<LocatedTriple>(std::cout, " "));
+            std::ostream_iterator<LocatedTriple>(os, " "));


You can use std::ranges::copy, then you don't need begin() and end()

joka921 · 2023-06-15T12:55:06Z

src/index/LocatedTriples.cpp

+  std::transform(ltpb.map_.begin(), ltpb.map_.end(),
+                 std::back_inserter(blockIndices),
+                 [](const auto& entry) { return entry.first; });


Suggested change

std::transform(ltpb.map_.begin(), ltpb.map_.end(),

std::back_inserter(blockIndices),

[](const auto& entry) { return entry.first; });

// TODO<C++23> use `std::ranges::to<std::vector>`

std::ranges::transform(ltpb.map_ | std::views::keys,

std::back_inserter(blockIndices));

(needs the <ranges> header.

joka921 · 2023-06-15T12:58:58Z

src/index/LocatedTriples.cpp

+// ____________________________________________________________________________
+std::ostream& operator<<(std::ostream& os, const IdTable& idTable) {
+  os << "{ ";
+  std::copy(idTable.begin(), idTable.end(),


use std::ranges::copy

joka921 · 2023-06-15T12:58:59Z

src/index/LocatedTriples.cpp

+// ____________________________________________________________________________
+std::ostream& operator<<(std::ostream& os,
+                         const columnBasedIdTable::Row<Id>& idTableRow) {
+  os << "(";
+  for (size_t i = 0; i < idTableRow.numColumns(); ++i) {
+    os << idTableRow[i] << (i < idTableRow.numColumns() - 1 ? " " : ")");
+  }
+  return os;
+}
+
+// ____________________________________________________________________________
+std::ostream& operator<<(std::ostream& os, const IdTable& idTable) {
+  os << "{ ";
+  std::copy(idTable.begin(), idTable.end(),
+            std::ostream_iterator<columnBasedIdTable::Row<Id>>(os, " "));
+  os << "}";
+  return os;
+}


I am not against implementing those, but they should be in the IdTable.h header if you need them.

joka921 · 2023-06-15T12:59:35Z

src/index/LocatedTriples.h

+std::ostream& operator<<(std::ostream& os,
+                         const columnBasedIdTable::Row<Id>& idTableRow);
+std::ostream& operator<<(std::ostream& os, const IdTable& idTable);


As stated above, those should be in the correct header.

test/LocatedTriplesTest.cpp

joka921 · 2023-06-15T13:13:32Z

test/LocatedTriplesTest.cpp

+  testWithGivenBlockSize(
+      triplesInIndex, triplesToLocate, 16,
+      {{2, "{ LT(2 0 V:2 V:14 V:20 0) LT(2 0 V:2 V:15 V:20 1) }"},
+       {4, "{ LT(4 0 V:2 V:20 V:10 1) }"},
+       {5, "{ LT(5 0 V:2 V:30 V:20 1) }"},
+       {6, "{ LT(6 0 V:2 V:30 V:30 1) }"},
+       {7, "{ LT(7 0 V:2 V:30 V:31 0) }"},
+       {8, "{ LT(8 x V:9 V:30 V:32 0) }"}});


I like the structure of these tests in general. I would, as already stated, strictly prefer if there were proper objects for testing and not strings, e.g.

using LT = LocatedTriple; LocatedTriplesMap expected {{2, {LT(2, 0, V(2), V(14), V(20, 0, LT{2, 0, V(2), V(15), V(20}}, ...}....

You can make this work in a typesafe way with a syntax that is still rather readable.

This PR is from June 2023. Since then, quite a bit of code that is relevant for this PR has been refactored. In particular: the `Permutation` class, the `CompressedRelationWriter` and the index building functions in `IndexImpl`. Mostly, this refactoring has made the code in this PR simpler. There is still some akwardness in `LocatedTriplesTest.cpp` because we want to build an index from `Id` triples there (and not from Turtle input). Now evertyhing compiles and runs through again. Various tests in `LocatedTriplesTest` fail, that's what I will look at next.

sonarqubecloud · 2024-02-10T09:40:30Z

Quality Gate passed

Issues
25 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

The code is based on the PRs ad-freiburg#916 and ad-freiburg#1000

Code for locating triples in an existing index

84a4bdf

This is the first part of a series of PRs split of from the large proof-of-concept PR #916, which realizes SPARQL 1.1 Update

hannahbast requested a review from joka921 June 9, 2023 14:50

joka921 requested changes Jun 9, 2023

View reviewed changes

Hannah Bast added 2 commits June 10, 2023 18:40

Add test for the locatedTriple method + address some of the comment…

d8781a4

…s from Johannes' review

Improve the locateTriple test (three relations instead of just one)

258231d

joka921 requested changes Jun 15, 2023

View reviewed changes

A few minor improvements

59aae8e

Qup42 added a commit to Qup42/qlever that referenced this pull request Apr 29, 2024

Initial adaption of LocatedTriples

4a145d1

The code is based on the PRs ad-freiburg#916 and ad-freiburg#1000

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code for locating triples in an existing index #1000

Code for locating triples in an existing index #1000

hannahbast commented Jun 9, 2023

codecov bot commented Jun 9, 2023 •

edited

Loading

joka921 left a comment

joka921 Jun 9, 2023

hannahbast Jun 9, 2023

joka921 Jun 9, 2023

joka921 Jun 9, 2023

joka921 Jun 9, 2023

joka921 Jun 9, 2023

joka921 Jun 9, 2023

sonarqubecloud bot commented Jun 10, 2023

joka921 left a comment

joka921 Jun 15, 2023

joka921 Jun 15, 2023

joka921 Jun 15, 2023

joka921 Jun 15, 2023

joka921 Jun 15, 2023

joka921 Jun 15, 2023

sonarqubecloud bot commented Feb 10, 2024

Code for locating triples in an existing index #1000

Are you sure you want to change the base?

Code for locating triples in an existing index #1000

Conversation

hannahbast commented Jun 9, 2023

codecov bot commented Jun 9, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jun 10, 2023

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

sonarqubecloud bot commented Feb 10, 2024

Quality Gate passed

codecov bot commented Jun 9, 2023 •

edited

Loading