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

fix(sipi): Improve performance of file value query #1697

Merged
merged 5 commits into from Sep 2, 2020

Conversation

@benjamingeer
Copy link
Collaborator

@benjamingeer benjamingeer commented Sep 1, 2020

https://dasch.myjetbrains.com/youtrack/issue/DSP-582

This PR rewrites the getFileValue SPARQL templates to use a CONSTRUCT query that seems to be more efficient with Jena than the current SELECT query is.

@benjamingeer benjamingeer requested a review from tobiasschweizer Sep 1, 2020
@benjamingeer benjamingeer self-assigned this Sep 1, 2020
benjamingeer added 2 commits Sep 1, 2020
Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

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 {

This comment has been minimized.

@tobiasschweizer

tobiasschweizer Sep 2, 2020
Contributor

@benjamingeer Could you test that with GraphDB locally?

This comment has been minimized.

@benjamingeer

benjamingeer Sep 2, 2020
Author Collaborator

This doesn't seem possible with the current Bazel setup.

This comment has been minimized.

@tobiasschweizer

tobiasschweizer Sep 2, 2020
Contributor

Ideally, there would be a config option.

This comment has been minimized.

@benjamingeer

benjamingeer Sep 2, 2020
Author Collaborator

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.

This comment has been minimized.

@tobiasschweizer

tobiasschweizer Sep 2, 2020
Contributor

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 .

This comment has been minimized.

@tobiasschweizer

tobiasschweizer Sep 2, 2020
Contributor

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.

This comment has been minimized.

@benjamingeer

benjamingeer Sep 2, 2020
Author Collaborator

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

This comment has been minimized.

@benjamingeer

benjamingeer Sep 2, 2020
Author Collaborator

I just realised that this means that the FILTER is redundant: 05029e9

This comment has been minimized.

@benjamingeer

benjamingeer Sep 2, 2020
Author Collaborator

(Values don't have attachedToProject anymore, either.)

This comment has been minimized.

@tobiasschweizer

tobiasschweizer Sep 2, 2020
Contributor

I just realised that this means that the FILTER is redundant: 05029e9

true, good catch!

…ct from file values.
Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

It's really fast now, thanks for this PR!

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Sep 2, 2020

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. *@

This comment has been minimized.

@tobiasschweizer

tobiasschweizer Sep 2, 2020
Contributor

that comment means that we know how the triplestore works ;-)

This comment has been minimized.

@benjamingeer

benjamingeer Sep 2, 2020
Author Collaborator

Exactly! :)

@benjamingeer benjamingeer merged commit 8214877 into develop Sep 2, 2020
7 checks passed
7 checks passed
Build Everything
Details
API Unit Tests
Details
API E2E Tests
Details
API Integration Tests
Details
Upgrade Integration Tests
Details
Docs Build Test
Details
Publish (on release only)
Details
@benjamingeer benjamingeer deleted the wip/DSP-582-file-value branch Sep 2, 2020
@subotic subotic added the enhancement label Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants