-
Notifications
You must be signed in to change notification settings - Fork 70
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
Adding support for named graphs #196
Conversation
…ot just BaseDictionary
Is this based on HDTQ (previous implementation: https://github.com/Rummy23/hdtq-java)? |
@bb010g Yes |
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.
The code is fine, I added some comments.
Adding some tests in the core would be better to make this new part reliable.
hdt-java-core/src/main/java/org/rdfhdt/hdt/compact/bitmap/Bitmap64Roaring.java
Outdated
Show resolved
Hide resolved
hdt-java-core/src/main/java/org/rdfhdt/hdt/compact/bitmap/Bitmap64Roaring.java
Outdated
Show resolved
Hide resolved
hdt-java-core/src/main/java/org/rdfhdt/hdt/compact/bitmap/Bitmap64Roaring.java
Outdated
Show resolved
Hide resolved
|
||
protected AdjacencyList adjY, adjZ, adjIndex; | ||
public AdjacencyList adjY, adjZ, adjIndex; |
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.
Is there a reason for this change?
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.
Yes, the class BitmapTriples is used by, for instance, BitmapQuadsIteratorZFOQ which needs to read these fields. This is done with a composition pattern, not inheritance, so protected won't work. The best I could to is creating getters (and setters maybe? didn't check if needed)
hdt-java-core/src/main/java/org/rdfhdt/hdt/triples/impl/BitmapTriplesIteratorYFOQ.java
Outdated
Show resolved
Hide resolved
hdt-java-core/src/main/java/org/rdfhdt/hdt/triples/impl/BitmapQuadTriples.java
Outdated
Show resolved
Hide resolved
Hi @QuentinJanuel , I saw that you added named graph cat test. This is fine, but what we would need more is the following:
this is the main test we need. You can ping me if it is not clear ... |
Sure no problem, I will prioritize these tests then |
thank you for the contribution and the reviews! |
This pull request will aim to address the issue #3