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(gravsearch): Allow comparing variables representing resource IRIs #1713
Conversation
…se of property IRIs, not property variables - Add tests. - Update design doc.
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.
Can you please add a test to the GravsearchTypeInspection to see what types are inferred for the following query?
val QueryWithFilterComparison: String =
"""
|PREFIX knora-api: <http://api.knora.org/ontology/knora-api/simple/v2#>
|PREFIX beol: <http://0.0.0.0:3333/ontology/0801/beol/simple/v2#>
|
|CONSTRUCT {
| ?person knora-api:isMainResource true .
| ?document beol:hasAuthor ?person .
|} WHERE {
| ?person a knora-api:Resource .
| ?person a beol:person .
|
| ?document beol:hasAuthor ?person .
| beol:hasAuthor knora-api:objectType knora-api:Resource .
| ?document a knora-api:Resource .
| FILTER(?document != "<aLetterIRI>")
|}
""".stripMargin
I would expect such a query to return manuscripts and letters written by that specific person but not one single letter with that particular IRI. So basically resources of type or subclass of beol:writtenSource
including beol:basicLetter
, beol:manuscript
, and beol:letter
.
I am afraid that the type inspection from the comparison in FILTER limits search results to resources of type beol:letter
.
test_data/all_data/beol-data.ttl
Outdated
knora-base:hasPermissions "CR knora-admin:Creator|V knora-admin:UnknownUser"; | ||
knora-base:attachedToUser <http://rdfh.ch/users/PSGbemdjZi4kQ6GHJVkLGF>; | ||
beol:title <http://rdfh.ch/0801/XNn6wanrTHWShGTjoULm5g/values/jGtT_87FS2qTkUEigdXMPg>; | ||
beol:hasSubject <http://rdfh.ch/0801/XNn6wanrTHWShGTjoULm5g/values/YdnovfaXRnW4c5qO7h02Ag>; |
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 the statement with a 'hasSubject' because if not this test letter will appear in the visualizations of the BEOL subject index and their respective letters.
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.
This is test data. Why would it be used on the BEOL live server?
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.
beol-data.ttl is not a pure test data. It contains subject index definition as list nodes, etc. The imported BEOL data and the data in this file are combined and made available on production server. for example, you can see that test letter defined in this file has ended up on Beol production server.
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.
I understand, but that doesn't seem right to me, because there are already fake letters in beol-data.ttl
. Wouldn't it be better to make two files, to separate the real data from the test data?
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.
I removed the hasSubject
in 99d13d1.
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.
I understand, but that doesn't seem right to me, because there are already fake letters in
beol-data.ttl
. Wouldn't it be better to make two files, to separate the real data from the test data?
It would have been, but never happened!
@@ -186,7 +186,7 @@ class InferringGravsearchTypeInspector(nextInspector: Option[GravsearchTypeInspe | |||
/** | |||
* Infers the `knora-api:objectType` of a property if the property's IRI is used as a predicate. | |||
*/ | |||
private class KnoraObjectTypeFromPropertyIriRule(nextRule: Option[InferenceRule]) extends InferenceRule(nextRule = nextRule) { | |||
private class InferTypeOfPropertyFromItsIri(nextRule: Option[InferenceRule]) extends InferenceRule(nextRule = nextRule) { |
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.
since it infers the objectType of a property, I believe the previous name was more clear. Maybe it's better to rename this to InferObjectTypeFromPropertyIri
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.
The problem here is to make it clear that we mean "infer type information about a property" and not "infer type information about an entity that is used as the object of a property". I think ObjectType
and KnoraObjectType
are ambiguous, because "object type" could mean "the type of the object of a property". I think TypeOfProperty
makes it clear that we're getting information about the property and not about its object. Does that make sense?
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.
ok, I see what you are trying to do. Yes, it makes sense.
*/ | ||
private class TypeOfSubjectFromPropertyRule(nextRule: Option[InferenceRule]) extends InferenceRule(nextRule = nextRule) { | ||
private class InferTypeOfSubjectFromPredicateIri(nextRule: Option[InferenceRule]) extends InferenceRule(nextRule = nextRule) { |
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.
To be consistent with the rule above shouldn't this be InferSubjectTypeFromPropertyIri
?
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.
I think SubjectType
is ambiguous: it could mean "infer the expected subject type of the property" or "infer the type of an entity that is used as the subject of a property". But TypeOfSubject
makes it clear that we're inferring information about a subject, not about a property.
@@ -365,12 +405,12 @@ class InferringGravsearchTypeInspector(nextInspector: Option[GravsearchTypeInspe | |||
/** | |||
* Infers the `knora-api:objectType` of a property variable or IRI if it's used with an object whose type is known. | |||
*/ | |||
private class KnoraObjectTypeFromObjectRule(nextRule: Option[InferenceRule]) extends InferenceRule(nextRule = nextRule) { | |||
private class InferTypeOfPredicateFromObject(nextRule: Option[InferenceRule]) extends InferenceRule(nextRule = nextRule) { |
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.
I believe the previous name was more clear, maybe we can rename this to 'InferObjectTypeFromObject'
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.
Same as above: I think ObjectType
is ambiguous.
@@ -2499,68 +2499,6 @@ class SearchRouteV2R2RSpec extends R2RSpec { | |||
} | |||
} | |||
|
|||
"do a Gravsearch query for a letter that links to a person with a specified name (optional)" in { |
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 have you removed these three tests?
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.
As explained in the description of this PR:
Remove some tests from
SearchRouteV2R2RSpec
usingOPTIONAL
, because they make it difficult to add new test data, and seem redundant anyway (theOPTIONAL
doesn't affect the results).
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.
The problem is that these tests are so vague that when you add new test data, the existing vague tests find the new data and this makes them fail.
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.
And because the OPTIONAL
has no effect on the results, those tests are the same as other existing tests without the OPTIONAL
.
It infers that GravsearchTypeInspectionResult(
entities = Map(
TypeableVariable(variableName = "person") -> NonPropertyTypeInfo(
typeIri = "http://0.0.0.0:3333/ontology/0801/beol/simple/v2#person".toSmartIri,
isResourceType = true,
isValueType = false
),
TypeableVariable(variableName = "document") -> NonPropertyTypeInfo(
typeIri = "http://api.knora.org/ontology/knora-api/simple/v2#Resource".toSmartIri,
isResourceType = true,
isValueType = false
),
TypeableIri(iri = "http://0.0.0.0:3333/ontology/0801/beol/simple/v2#hasAuthor".toSmartIri) -> PropertyTypeInfo(
objectTypeIri = "http://0.0.0.0:3333/ontology/0801/beol/simple/v2#person".toSmartIri,
objectIsResourceType = true,
objectIsValueType = false
),
TypeableIri(iri = "http://rdfh.ch/0801/XNn6wanrTHWShGTjoULm5g".toSmartIri) -> NonPropertyTypeInfo(
typeIri = "http://api.knora.org/ontology/knora-api/simple/v2#Resource".toSmartIri,
isResourceType = true,
isValueType = false
)
),
entitiesInferredFromProperties = Map(
TypeableVariable(variableName = "document") -> Set(NonPropertyTypeInfo(
typeIri = "http://api.knora.org/ontology/knora-api/simple/v2#Resource".toSmartIri,
isResourceType = true,
isValueType = false
)),
TypeableVariable(variableName = "person") -> Set(NonPropertyTypeInfo(
typeIri = "http://0.0.0.0:3333/ontology/0801/beol/simple/v2#person".toSmartIri,
isResourceType = true,
isValueType = false
))
)
) This is because |
Test added: db9b92c |
Thanks for the review! |
Thanks for the quick fix! |
https://dasch.myjetbrains.com/youtrack/issue/DSP-656
InferringGravsearchTypeInspector
:FILTER
expressions, and add a rule (InferTypeOfEntityFromComparisonWithOtherEntityInFilter
) that infers the types of those entities from those comparisons.visitFilterExpression
because it was getting too long and hard to read.InferTypeOfObjectFromPredicate
: If we have a statement like?letter ?prop ?person1
, where the property is a variable, and we have already inferred its type, and then we use that to infer the type of?person1
, don't use that as a reason to optimise away?person1 rdf:type ...
later, because in the triplestore, a pattern like?letter ?prop ?person1
could match any statement.AbstractPrequeryGenerator.handleQueryVar()
, allow aFILTER
expression comparing two entities representing resources.SearchRouteV2R2RSpec
usingOPTIONAL
, because they make it difficult to add new test data, and seem redundant anyway (theOPTIONAL
doesn't affect the results).Closes #1441.