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(gravsearch): Allow comparing variables representing resource IRIs #1713

Merged
merged 7 commits into from Sep 22, 2020

Conversation

benjamingeer
Copy link

@benjamingeer benjamingeer commented Sep 18, 2020

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

  • In InferringGravsearchTypeInspector:
    • Index variables that are compared with other variables or IRIs in FILTER expressions, and add a rule (InferTypeOfEntityFromComparisonWithOtherEntityInFilter) that infers the types of those entities from those comparisons.
      • Refactor visitFilterExpression because it was getting too long and hard to read.
      • Rename all the inference rules and refactor them a bit for clarity.
    • In the rule 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.
  • In AbstractPrequeryGenerator.handleQueryVar(), allow a FILTER expression comparing two entities representing resources.
  • Add tests.
    • Remove some tests from SearchRouteV2R2RSpec using OPTIONAL, because they make it difficult to add new test data, and seem redundant anyway (the OPTIONAL doesn't affect the results).
  • Add docs.

Closes #1441.

@benjamingeer benjamingeer added enhancement improve existing code or new feature API/V2 labels Sep 18, 2020
@benjamingeer benjamingeer self-assigned this Sep 18, 2020
@benjamingeer benjamingeer marked this pull request as draft September 18, 2020 09:29
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.

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.

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

Copy link
Author

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?

Copy link
Contributor

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.

Copy link
Author

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?

Copy link
Author

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.

Copy link
Contributor

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

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

Copy link
Author

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?

Copy link
Contributor

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

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?

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

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'

Copy link
Author

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

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?

Copy link
Author

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 using OPTIONAL, because they make it difficult to add new test data, and seem redundant anyway (the OPTIONAL doesn't affect the results).

Copy link
Author

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.

Copy link
Author

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.

@benjamingeer
Copy link
Author

benjamingeer commented Sep 22, 2020

Can you please add a test to the GravsearchTypeInspection to see what types are inferred for the following query?

It infers that ?document is a knora-api:Resource:

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 beol:hasAuthor is a subproperty of knora-base:hasLinkTo, whose knora-base:subjectClassConstraint is knora-base:Resource. There's nothing in the query that allows the type beol:letter to be inferred. The inferring type inspector doesn't know that your letter IRI refers to a beol:letter; it just knows that it's a resource IRI.

@benjamingeer
Copy link
Author

Test added: db9b92c

@benjamingeer benjamingeer marked this pull request as ready for review September 22, 2020 12:40
@benjamingeer
Copy link
Author

Thanks for the review!

@SepidehAlassi
Copy link
Contributor

Thanks for the review!

Thanks for the quick fix!

@benjamingeer benjamingeer merged commit f359c8e into develop Sep 22, 2020
@benjamingeer benjamingeer deleted the wip/DSP-656-gravsearch branch September 22, 2020 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API/V2 enhancement improve existing code or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't compare two resources by IRI in Gravsearch
2 participants