-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SPARQL Update Part 1: Located Triples #1379
base: master
Are you sure you want to change the base?
Conversation
This PR will run somewhat parallel to #1351. To keep it in sync I will cherry-pick the changes made here back to it. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1379 +/- ##
==========================================
+ Coverage 88.89% 88.98% +0.08%
==========================================
Files 327 332 +5
Lines 28974 29370 +396
Branches 3210 3258 +48
==========================================
+ Hits 25756 26134 +378
- Misses 2066 2073 +7
- Partials 1152 1163 +11 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some preliminary comments, haven't looked at everything yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some more comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some additional suggestions and discussions.
src/global/IdTriple.h
Outdated
// TODO<qup42>: should this be a `PermutedTriple`? | ||
IdTriple permute(const std::array<size_t, 3>& keyOrder) const { | ||
return IdTriple{ids_[keyOrder[0]], ids_[keyOrder[1]], ids_[keyOrder[2]]}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is in fact a REALLY good idea.
We should have Triples where the order is subject, predicate, object (and the member are maybe even called as such).
And then we have permuted triples that always belong to a permutation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applying this somewhat consistently would entail quite a lot more changes. To not loose the payload, PermutedTriple
would also have to get one. We'd want constructors to convert between them and (maybe comparators). For this PermutedTriple
should be extracted to a separate header and both would require header+implementation. src/global
would be a good, but currently only has headers... Not to speak of using it. I'll look how the need develops for the rest but, I think that this could be too much for a single PR.
@joka921 whats the usual procedure for the coverage of debugging |
|
This is the first PR in a series of PRs that will come from #1351.
Changes:
LocatedTriple
is a triple that is to be deleted/inserted and has been assigned to a block in the index.locateTriplesInPermutation
LocatedTriples
are an ordered set ofLocatedTriple
s.LocatedTriplesPerBlock
areLocatedTriples
for different blocks. This data structure will later exist once per Permutation.LocatedTriple
s:add
anderase
IdTable
by the changes:mergeTriples