-
Notifications
You must be signed in to change notification settings - Fork 18
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
fix(sipi): Improve performance of file value query #1697
Conversation
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.
It's really fast now! :-)
@@ -38,40 +35,24 @@ PREFIX rdf: <http://www.w3.org/1999/02/22-rdf-syntax-ns#> | |||
PREFIX rdfs: <http://www.w3.org/2000/01/rdf-schema#> | |||
PREFIX knora-base: <http://www.knora.org/ontology/knora-base#> | |||
|
|||
SELECT ?fileValue ?objPred ?objObj | |||
CONSTRUCT { |
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.
@benjamingeer Could you test that with GraphDB locally?
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.
This doesn't seem possible with the current Bazel setup.
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.
Ideally, there would be a config option.
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.
I think our policy at the moment is that we're not officially maintaining support for GraphDB. But I'm trying to do it anyway if it's not too much effort.
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.
maybe at some point we might have more than one free triplestore available
knora-base:isDeleted false . | ||
|
||
?prop rdfs:subPropertyOf* knora-base:hasFileValue . | ||
knora-base:hasPermissions ?currentFileValuePermissions . |
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.
I am not sure I remember why this is necessary (although I seem to have worked on that query): I suppose it's necessary because permissions are only stored for the current version of a value, and if someone asks for a filename that was changed with a later version of the file value, it's the only way to get the permissions, right?
So this means permissions are not versioned.
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 permissions attached to the current version of a value also apply to previous versions of the value. Value versions other than the current one do not have this predicate.
https://docs.knora.org/02-knora-ontologies/knora-base/#permissions
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.
I just realised that this means that the FILTER
is redundant: 05029e9
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.
(Values don't have attachedToProject
anymore, either.)
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.
I just realised that this means that the
FILTER
is redundant: 05029e9
true, good catch!
…ct from file values.
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.
It's really fast now, thanks for this PR!
Thanks for the review and for testing it! |
@@ -50,6 +50,10 @@ CONSTRUCT { | |||
knora-base:attachedToProject ?resourceProject . | |||
|
|||
?fileValue ?objPred ?objObj . | |||
|
|||
@* This FILTER is unnecessary, but it makes Jena run the query faster. *@ |
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 comment means that we know how the triplestore works ;-)
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.
Exactly! :)
https://dasch.myjetbrains.com/youtrack/issue/DSP-582
This PR rewrites the
getFileValue
SPARQL templates to use aCONSTRUCT
query that seems to be more efficient with Jena than the currentSELECT
query is.