IndexScan only has LocatedTriples for the relevant permutation
#2618
Conversation
Qup42
left a comment
Qup42
left a comment
There was a problem hiding this comment.
Some places still requiring a bit of polishing.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2618 +/- ##
==========================================
+ Coverage 91.50% 91.51% +0.01%
==========================================
Files 478 479 +1
Lines 41066 41182 +116
Branches 5464 5474 +10
==========================================
+ Hits 37578 37689 +111
- Misses 1909 1915 +6
+ Partials 1579 1578 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
RobinTF
left a comment
RobinTF
left a comment
There was a problem hiding this comment.
Some requests from my side, and please incorporate and/or remove your own TODO proposals.
# Conflicts: # src/engine/CountAvailablePredicates.cpp # src/engine/GroupByImpl.cpp # src/engine/HasPredicateScan.cpp # src/engine/IndexScan.cpp # src/engine/IndexScan.h # src/engine/MaterializedViews.cpp # src/engine/MaterializedViews.h # src/engine/QueryPlanner.cpp # src/index/IndexImpl.cpp # src/index/Permutation.cpp # src/index/Permutation.h # test/CompressedRelationsTest.cpp # test/IndexTest.cpp # test/MaterializedViewsTest.cpp
RobinTF
left a comment
RobinTF
left a comment
There was a problem hiding this comment.
Looking very good already in my opinion. Just some minor suggestions left from my side.
| const SparqlTripleSimple& triple) { | ||
| // Create alias shared pointer of internal the right `LocatedTriplesPerBlock`. | ||
| const auto& locatedTriples = | ||
| containsInternalIri(triple) |
There was a problem hiding this comment.
We have the exact same check in getPermutationForTriple. Is there a place where only one of the two functions is used? In my imagination you could simply put this return statement you have here and put it in the other function.
On the other hand separation of concerns is also a valid concern.
# Conflicts: # src/engine/CMakeLists.txt # src/engine/Server.cpp
Overview
Conformance check passed ✅No test result changes. |
|
joka921
left a comment
joka921
left a comment
There was a problem hiding this comment.
I have serious questions about the unwanted changes in unrelated files (where I liked the old syntax better). Maybe let's talk about this quickly.
| const auto& locatedTriples = | ||
| locatedTriplesState().getLocatedTriplesForPermutation<false>(perm); | ||
| auto fullHasPattern = index.getPermutation(perm).lazyScan( | ||
| CompressedRelationReader::ScanSpecAndBlocks::withUpdates(scanSpec, | ||
| locatedTriples), |
There was a problem hiding this comment.
I really don't like this pattern (you have to manually get the correct permutation twice, which is an API regression.
This should be done by a single place at once!
| locatedTriplesState().getLocatedTriplesForPermutation<false>( | ||
| permutationEnum.value()), |
There was a problem hiding this comment.
same here.
If the reason for this is,
that you with your new changes sometimes only already have the located triples for a single permutation, that probably we can have overloads in the Permutation class, instead of making this error-prone and verbose update to all the classes using the Permutation class.
3 participants
No description provided.