Skip to content
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

Merge located triples when performing index scans #1597

Merged
merged 19 commits into from
Nov 14, 2024

Conversation

joka921
Copy link
Member

@joka921 joka921 commented Oct 31, 2024

PR #1582 and #1603 gave all index-lookup methods access to a snapshot of the (located) delta triples. With this change, these triples are now merged with the original triples during query processing whenever necessary. When an index block does not contain any located triples, the performance for accessing that block is the same as before.

The methods for obtaining the result size of an index scan now have two versions: one for obtaining an approximate size (this is cheap because it can be computed from the metadata of the blocks and the located triples) and one for obtaining the exact size (if there are located triples this is expensive because it requires reading and decompressing a block and merging the located triples).

TODO<joka921>
Test the merging of the located triples.

Signed-off-by: Johannes Kalmbach <[email protected]>
Check for code coverage/self-review/etc.

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

codecov bot commented Oct 31, 2024

Codecov Report

Attention: Patch coverage is 95.47038% with 13 lines in your changes missing coverage. Please review.

Project coverage is 89.25%. Comparing base (1bcfeeb) to head (beb1326).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/index/CompressedRelation.cpp 96.15% 5 Missing and 4 partials ⚠️
src/engine/Join.cpp 70.00% 2 Missing and 1 partial ⚠️
src/engine/IndexScan.h 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1597      +/-   ##
==========================================
+ Coverage   89.21%   89.25%   +0.04%     
==========================================
  Files         372      372              
  Lines       34723    34970     +247     
  Branches     3915     3942      +27     
==========================================
+ Hits        30979    31214     +235     
- Misses       2471     2485      +14     
+ Partials     1273     1271       -2     

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

Hannah Bast and others added 6 commits November 10, 2024 19:38
Signed-off-by: Johannes Kalmbach <[email protected]>
Signed-off-by: Johannes Kalmbach <[email protected]>
TODO: `DeltaTriplesTest, DeltaTriplesManager` failed because
`locatedSPO.containsTriple` was removed. I have commented out that code
for now, but this needs to be fixed properly.
Signed-off-by: Johannes Kalmbach <[email protected]>
Copy link
Member

@hannahbast hannahbast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Another minor revsion by you and me and we are done

Copy link
Member

@hannahbast hannahbast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great now. I will make another quick pass later, and if I don't find anything, will merge this

Signed-off-by: Johannes Kalmbach <[email protected]>
@sparql-conformance
Copy link

@hannahbast
Copy link
Member

@joka921 I will write the description for this PR tomorrow (Thursday). One more thing: SonarCloud identified 10 issues: do we want to let these go or are there some of them, which you still want to address for this PR?

@hannahbast hannahbast changed the title Merge in the located triples when performing index scans. Merge located triples when performing index scans Nov 14, 2024
Copy link
Member

@hannahbast hannahbast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again + I will merge this now!

@hannahbast hannahbast merged commit 77ac964 into ad-freiburg:master Nov 14, 2024
22 checks passed
realHannes pushed a commit to realHannes/qlever that referenced this pull request Nov 15, 2024
PR ad-freiburg#1582 and ad-freiburg#1603 gave all index-lookup methods access to a snapshot of the (located) delta triples. With this change, these triples are now merged with the original triples during query processing whenever necessary. When an index block does not contain any located triples, the performance for accessing that block is the same as before.

The methods for obtaining the result size of an index scan now have two versions: one for obtaining an approximate size (this is cheap because it can be computed from the metadata of the blocks and the located triples) and one for obtaining the exact size (if there are located triples this is expensive because it requires reading and decompressing a block and merging the located triples).
@joka921 joka921 deleted the merge-located-triples branch December 18, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants