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

Gravsearch search for list value using complex schema #899

Merged
merged 21 commits into from Jun 25, 2018

Conversation

benjamingeer
Copy link

@benjamingeer benjamingeer commented Jun 19, 2018

  • Support existing query functionality using the complex schema.
  • Check that if the complex schema is used, the simple schema is allowed only in date comparisons in FILTER expressions.
  • Support searching for list values with particular list nodes using the complex schema.
  • Reject properties that don't exist in the schema in which they're submitted.
  • Don't allow entities from rdf, rdfs, or owl in a Gravsearch CONSTRUCT clause.
  • Reject properties that aren't (yet) supported in Gravsearch.
  • Type inference improvements:
    • Don't run a rule more than once if it can never return new information after the first iteration.
    • Infer the type of a variable used in a function.
  • Add tests.
  • Update docs.
  • Update release notes.

Resolves #813.

@benjamingeer benjamingeer added enhancement improve existing code or new feature API/V2 Gravsearch labels Jun 19, 2018
@benjamingeer benjamingeer self-assigned this Jun 19, 2018
Benjamin Geer added 15 commits June 19, 2018 18:10
- Add pathological type inference test.
- Add sanity check for #904.
- Remove unused SPARQL query template.
…alues.

- Add a test that searches for a list node.
- Add missing properties to knora-api v2 complex.
- Don't allow the use of knora-api properties that aren't in the specified schema.
- Forbid more knora-api complex properties in Gravsearch.
@benjamingeer
Copy link
Author

@tobiasschweizer I disallowed rdf:type in Gravsearch CONSTRUCT clauses, because it actually serves no purpose there. Will that require a change in SALSAH? Also, could you maybe add a more realistic test with a list value from BEOL?

@tobiasschweizer
Copy link
Contributor

@benjamingeer I do not think so, but I will check on Monday!

@tobiasschweizer
Copy link
Contributor

I disallowed rdf:type in Gravsearch CONSTRUCT clauses, because it actually serves no purpose there. Will that require a change in SALSAH?

I checked the Gravsearch query templates in SALSAH and they work with this branch.

Also, could you maybe add a more realistic test with a list value from BEOL?

You mean a v2 search route test for a beol list value? Sure, I will do that now.

@benjamingeer
Copy link
Author

You mean a v2 search route test for a beol list value? Sure, I will do that now.

Yes, thanks.

@tobiasschweizer
Copy link
Contributor

@benjamingeer and here you go: 81643cd

@benjamingeer
Copy link
Author

@tobiasschweizer Great, thank you! Would you have time to finish reviewing this today?


#### Functions in FILTER Expressions
A Knora value may not be represented as the literal object of a predicate;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should see how hard it is to allow literals in the simple schema. The main reason why we forbade this was because it makes the processing harder in SearchResponderV2.

In simple schema, we have to add an extra level by generating a statement for valueHasString. However, from the user's perspective it is rather hard to understand why Gravsearch supports literals in complex schema while it does not in simple schema.

Copy link
Author

Choose a reason for hiding this comment

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

If you used a literal instead of a variable, that would mean you couldn't mention that value in the CONSTRUCT clause, so wouldn't it be excluded from the results? I think this is rarely what the user would want.

Copy link
Author

Choose a reason for hiding this comment

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

By the way, the rule is currently the same for both schemas: you can't represent a Knora value as the literal object of a predicate. In the complex schema, you can use literals for things below the value level, but not for the value itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you used a literal instead of a variable, that would mean you couldn't mention that value in the CONSTRUCT clause, so wouldn't it be excluded from the results?

I think in that case you would just use the same statement:

PREFIX beol: <http://0.0.0.0:3333/ontology/0801/beol/simple/v2#>
PREFIX knora-api: <http://api.knora.org/ontology/knora-api/simple/v2#>
PREFIX xsd: <http://www.w3.org/2001/XMLSchema#>

    CONSTRUCT {
        ?letter knora-api:isMainResource true .

        ?letter beol:creationDate ?date .

        ?letter ?linkingProp1  ?person1 .

        ?person1 beol:hasFamilyName "Meier" .

    } WHERE {
      
        ?letter a beol:letter .

        ?letter beol:creationDate ?date .

        ?letter ?linkingProp1 ?person1 .
        FILTER(?linkingProp1 = beol:hasAuthor || ?linkingProp1 = beol:hasRecipient )

        ?person1 beol:hasFamilyName "Meier".

    } ORDER BY ?date

I think in a first version of Gravsearch, we allowed for literals. But when I refactored it, I did not support them anymore because the processing logic would have been more complex.

In webapi/src/test/scala/org/knora/webapi/e2e/v2/SearchRouteV2R2RSpec.scala there are still some old tests involving literals that are ignored.

Copy link
Author

Choose a reason for hiding this comment

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

OK, let's look at this after standoff search is done.

@tobiasschweizer
Copy link
Contributor

Would you have time to finish reviewing this today?

I am doing it now :-)

@tobiasschweizer
Copy link
Contributor

OK, let's look at this after standoff search is done.

Yes, absolutely! I do not think it is urgent since you can do everything with a Filter that you can do with a literal.

case _ =>
// value property
} else {
// Is the subject a resource?
Copy link
Contributor

Choose a reason for hiding this comment

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

If we get here, it is not a linking property, right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. Clarified the comments there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@@ -2216,22 +2178,25 @@ class SearchResponderV2 extends ResponderWithStandoffV2 {
typeInspectionResult: GravsearchTypeInspectionResult <- gravsearchTypeInspectionRunner.inspectTypes(inputQuery.whereClause, requestingUser)
whereClauseWithoutAnnotations: WhereClause = GravsearchTypeInspectionUtil.removeTypeAnnotations(inputQuery.whereClause)

// Preprocess the query to convert API IRIs to internal IRIs and to set inference per statement.

preprocessedQuery: ConstructQuery = QueryTraverser.transformConstructToConstruct(
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you get rid of this because in the old way, we just converted everything to internal assuming that we got it in simple schema. Now, we have to check if we get it in simple or complex.

Copy link
Author

Choose a reason for hiding this comment

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

Yes.

<rdfs:subClassOf rdf:nodeID="genid-55a441049d76415ba3999f7d47ee542f-b13"/>
<rdfs:subClassOf rdf:nodeID="genid-55a441049d76415ba3999f7d47ee542f-b14"/>
<rdfs:subClassOf rdf:nodeID="genid-55a441049d76415ba3999f7d47ee542f-b15"/>
<rdfs:subClassOf rdf:nodeID="genid-0e6b49fe8a374047959ae8a349b1a4de-b8"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these blank node Iris?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, for cardinalities.

Copy link
Contributor

Choose a reason for hiding this comment

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

But aren't those different for every request you make?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. Every time you convert a JSON-LD document containing blank nodes to RDF/XML, you get different randomly generated blank node IDs. Semantically it makes no difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just wondered how the tests work. I assume the blank node Iris are just ignored, right?

Copy link
Author

Choose a reason for hiding this comment

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

The tests use RDF4J Rio to parse the RDF/XML into an org.eclipse.rdf4j.model.Model. If you compare two Model objects that contain blank nodes using the == operator, the Model class understands that the blank node IDs aren't significant. It just compares their contents, not their IDs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, got it. Thanks!

@benjamingeer benjamingeer merged commit d2140a8 into develop Jun 25, 2018
@benjamingeer benjamingeer deleted the wip/gravsearch-complex-schema branch June 25, 2018 13:06
@benjamingeer benjamingeer mentioned this pull request Sep 13, 2018
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 Gravsearch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants