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

@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 August 28, 2020 14:31
@benjamingeer benjamingeer marked this pull request as ready for review August 31, 2020 13:15
val iriStr = iri.toString
iriStr == OntologyConstants.KnoraApiV2Simple.Resource || iriStr == OntologyConstants.KnoraApiV2Complex.Resource
}
lazy val KnoraApiV2ResourceIris: Set[IRI] = Set(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Thanks for this, please see my comments.

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

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

Choose a reason for hiding this comment

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

please remove this log

Copy link
Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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
@benjamingeer benjamingeer deleted the wip/DSP-534-gravsearch-inference branch September 1, 2020 13:20
@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