-
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
feat(api-v2): Make inference optional in Gravsearch #1696
Conversation
- Clean up code a bit.
- Add docs.
- Add docs.
val iriStr = iri.toString | ||
iriStr == OntologyConstants.KnoraApiV2Simple.Resource || iriStr == OntologyConstants.KnoraApiV2Complex.Resource | ||
} | ||
lazy val KnoraApiV2ResourceIris: Set[IRI] = Set( |
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 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.
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.
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.
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.
Thanks for this, please see my comments.
triplestoreSpecificPrequery = QueryTraverser.transformSelectToSelect( | ||
inputQuery = nonTriplestoreSpecificPrequery, | ||
transformer = triplestoreSpecificQueryPatternTransformerSelect | ||
) | ||
|
||
// _ = println(triplestoreSpecificPrequery.toSparql) | ||
triplestoreSpecificPrequerySparql = triplestoreSpecificPrequery.toSparql | ||
_ = log.debug(triplestoreSpecificPrequerySparql) |
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.
please remove this log
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.
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) |
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.
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" |
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.
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
.
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.
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:
I've clarified this in a7f3a1c.
https://dasch.myjetbrains.com/youtrack/issue/DSP-534
AbstractPrequeryGenerator
, parseknora-api:GravsearchOptions knora-api:useInference
with a boolean object.knora-api:GravsearchOptions
andknora-api:useInference
in type inspection.SparqlTransformer
, if that option is set, change the way the generated prequery is processed:FROM <http://www.ontotext.com/explicit>
.