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
Collaborator

@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 self-assigned this Sep 18, 2020
@benjamingeer benjamingeer marked this pull request as draft Sep 18, 2020
…se of property IRIs, not property variables

- Add tests.
- Update design doc.
- Add tests.
@benjamingeer benjamingeer requested a review from SepidehAlassi Sep 21, 2020
Copy link
Contributor

@SepidehAlassi SepidehAlassi left a comment

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

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Sep 22, 2020
Contributor

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.

This comment has been minimized.

@benjamingeer

benjamingeer Sep 22, 2020
Author Collaborator

This is test data. Why would it be used on the BEOL live server?

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Sep 22, 2020
Contributor

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.

This comment has been minimized.

@benjamingeer

benjamingeer Sep 22, 2020
Author Collaborator

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?

This comment has been minimized.

@benjamingeer

benjamingeer Sep 22, 2020
Author Collaborator

I removed the hasSubject in 99d13d1.

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Sep 22, 2020
Contributor

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) {

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Sep 22, 2020
Contributor

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

This comment has been minimized.

@benjamingeer

benjamingeer Sep 22, 2020
Author Collaborator

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?

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Sep 22, 2020
Contributor

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) {

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Sep 22, 2020
Contributor

To be consistent with the rule above shouldn't this be InferSubjectTypeFromPropertyIri?

This comment has been minimized.

@benjamingeer

benjamingeer Sep 22, 2020
Author Collaborator

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) {

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Sep 22, 2020
Contributor

I believe the previous name was more clear, maybe we can rename this to 'InferObjectTypeFromObject'

This comment has been minimized.

@benjamingeer

benjamingeer Sep 22, 2020
Author Collaborator

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 {

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Sep 22, 2020
Contributor

why have you removed these three tests?

This comment has been minimized.

@benjamingeer

benjamingeer Sep 22, 2020
Author Collaborator

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

This comment has been minimized.

@benjamingeer

benjamingeer Sep 22, 2020
Author Collaborator

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.

This comment has been minimized.

@benjamingeer

benjamingeer Sep 22, 2020
Author Collaborator

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
Collaborator Author

@benjamingeer 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
Collaborator Author

@benjamingeer benjamingeer commented Sep 22, 2020

Test added: db9b92c

@benjamingeer benjamingeer marked this pull request as ready for review Sep 22, 2020
@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Sep 22, 2020

Thanks for the review!

@SepidehAlassi
Copy link
Contributor

@SepidehAlassi SepidehAlassi commented Sep 22, 2020

Thanks for the review!

Thanks for the quick fix!

@benjamingeer benjamingeer merged commit f359c8e into develop Sep 22, 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-656-gravsearch branch Sep 22, 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.

2 participants