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

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.

Copy link
Contributor

@tobiasschweizer tobiasschweizer left a 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 {
Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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 .
Copy link
Contributor

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.

Copy link
Author

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

Copy link
Author

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

Copy link
Author

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.)

Copy link
Contributor

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!

Copy link
Contributor

@tobiasschweizer tobiasschweizer left a 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!

@benjamingeer
Copy link
Author

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. *@
Copy link
Contributor

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 ;-)

Copy link
Author

Choose a reason for hiding this comment

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

Exactly! :)

@benjamingeer benjamingeer merged commit 8214877 into develop Sep 2, 2020
@benjamingeer benjamingeer deleted the wip/DSP-582-file-value branch September 2, 2020 16:12
@subotic subotic added the enhancement improve existing code or new feature label Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improve existing code or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants