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

feat(api-v2): Make inference optional in Gravsearch #1696

Merged
merged 10 commits into from Sep 1, 2020

Conversation

@benjamingeer
Copy link
Collaborator

@benjamingeer benjamingeer commented Aug 28, 2020

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

  • In AbstractPrequeryGenerator, parse knora-api:GravsearchOptions knora-api:useInference with a boolean object.
  • Ignore knora-api:GravsearchOptions and knora-api:useInference in type inspection.
  • In SparqlTransformer, if that option is set, change the way the generated prequery is processed:
    • With Fuseki, don't simulate inference by expanding statements.
    • With GraphDB, add FROM <http://www.ontotext.com/explicit>.
  • Add tests.
  • Add docs.
@benjamingeer benjamingeer self-assigned this Aug 28, 2020
@benjamingeer benjamingeer marked this pull request as draft Aug 28, 2020
- Clean up code a bit.
- Add docs.
- Add docs.
@benjamingeer benjamingeer requested a review from SepidehAlassi Aug 31, 2020
@benjamingeer benjamingeer marked this pull request as ready for review Aug 31, 2020
val iriStr = iri.toString
iriStr == OntologyConstants.KnoraApiV2Simple.Resource || iriStr == OntologyConstants.KnoraApiV2Complex.Resource
}
lazy val KnoraApiV2ResourceIris: Set[IRI] = Set(

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Sep 1, 2020
Contributor

@benjamingeer question: why are using lazy val here in the following three cases? The initialization does not need any evaluation of an expression, it is just a set. In this case, it would not make any difference if the initialization happens immediately or is postponed to the first time the value is accessed.

This comment has been minimized.

@benjamingeer

benjamingeer Sep 1, 2020
Author Collaborator

Great question! The problem is that KnoraApiV2ResourceIris is a static value (it's defined in an object rather than in a class), which refers to static values in another object. The order of initialisation of static values in the JVM depends on which object is loaded first by the JVM's class loader at run time. Without the lazy, if object KnoraApi is initialised before object KnoraApiV2Simple and object KnoraApiV2Complex, KnoraApiV2ResourceIris will just contain null values. It took me a little while to figure this out yesterday. A test was failing, and it turned out that it was because KnoraApiV2ResourceIris did not contain OntologyConstants.KnoraApiV2Complex.Resource, because it hadn't been initialised properly.

In the earlier code, there was a method, which didn't access those constants until it was called, so it was fine.

Copy link
Contributor

@SepidehAlassi SepidehAlassi left a comment

Thanks for this, please see my comments.

triplestoreSpecificPrequery = QueryTraverser.transformSelectToSelect(
inputQuery = nonTriplestoreSpecificPrequery,
transformer = triplestoreSpecificQueryPatternTransformerSelect
)

// _ = println(triplestoreSpecificPrequery.toSparql)
triplestoreSpecificPrequerySparql = triplestoreSpecificPrequery.toSparql
_ = log.debug(triplestoreSpecificPrequerySparql)

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Sep 1, 2020
Contributor

please remove this log

This comment has been minimized.

@benjamingeer

benjamingeer Sep 1, 2020
Author Collaborator

Why? It should have no effect unless debug logging is enabled. Using log seems better than using println, because with println, you have to uncomment it and recompile if you want to use it. With log, you just have to change the logging config file.

// println("++++++++")
// println(triplestoreSpecificSparql)
val triplestoreSpecificMainQuerySparql: String = triplestoreSpecificMainQuery.toSparql
log.debug(triplestoreSpecificMainQuerySparql)

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Sep 1, 2020
Contributor

please remove log

* @param namedGraph the named graph to be used in the query.
*/
case class FromClause(namedGraph: IriRef) extends SparqlGenerator {
override def toSparql: String = s"FROM ${namedGraph.toSparql}\n"

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Sep 1, 2020
Contributor

we have a named graph here, shouldn't we use FROM NAMED? Or are you using FROM because the namedGraph is anyway the default graph? In that case, please rename the nameGraph to defaultGraph.

This comment has been minimized.

@benjamingeer

benjamingeer Sep 1, 2020
Author Collaborator

Sorry, this is confusing. For me, any graph in the triplestore that has a name is a "named graph". The pseudo-graph <http://www.ontotext.com/explicit> is a "named graph" in that sense:

http://graphdb.ontotext.com/documentation/standard/query-behaviour.html#how-to-query-explicit-and-implicit-statements

I've clarified this in a7f3a1c.

@benjamingeer benjamingeer merged commit 166a260 into develop Sep 1, 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-534-gravsearch-inference branch Sep 1, 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