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 AND expression failing #846

Closed
SepidehAlassi opened this issue May 9, 2018 · 6 comments
Closed

Gravsearch AND expression failing #846

SepidehAlassi opened this issue May 9, 2018 · 6 comments
Assignees
Labels
API/V2 enhancement improve existing code or new feature
Milestone

Comments

@SepidehAlassi
Copy link
Contributor

Example:
In query below FILTER(?text = "Bonjour" && lang(?text) = "fr") is failing

"""
|PREFIX knora-api: http://api.knora.org/ontology/knora-api/simple/v2#
|PREFIX anything: http://0.0.0.0:3333/ontology/0001/anything/simple/v2#
|
|CONSTRUCT {
| ?thing knora-api:isMainResource true .
|
| ?thing a anything:Thing .
|
| ?thing anything:hasText ?text .
|} WHERE {
| ?thing a knora-api:Resource .
|
| ?thing a anything:Thing .
|
| ?thing anything:hasText ?text .
|
| anything:hasText knora-api:objectType xsd:string .
|
| ?text a xsd:string .
|
| FILTER(?text = "Bonjour" && lang(?text) = "fr")
|}
""".

@tobiasschweizer tobiasschweizer added enhancement improve existing code or new feature API/V2 labels May 9, 2018
@benjamingeer
Copy link

I think we can do it like this:

My understanding is that lang(?text) = "fr" gets replaced by some statements with no FILTER expression; the original expression becomes a placeholder None. So after this is done, we can traverse the expression tree, simplifying subexpressions that contain None as a branch. For example, if we find an AND expression with None as a branch, we replace the whole AND expression with the other branch.

@benjamingeer benjamingeer changed the title knarql AndExpression failing KnarQL And Expression failing May 9, 2018
@benjamingeer benjamingeer changed the title KnarQL And Expression failing Gravsearch AND expression failing May 30, 2018
@benjamingeer
Copy link

benjamingeer commented May 31, 2018

The way the filter statements are generated at the moment for lang, this can't work. Imagine this filter in Gravsearch:

FILTER(lang(?text) = "fr" || lang(?text) = "en")

This will generate additional statements to check for both conditions, and both sides of the expression will be removed. But all those statements must then match to get a search result, so in effect the OR becomes an AND, which is wrong. Similarly imagine this:

FILTER((?text = "maison" && lang(?text) = "fr") || (?text = "house" && lang(?text) = "en"))

This is valid SPARQL, but in Gravsearch it could never return any results.

The solution is not to remove the expression from the FILTER, but instead to replace it with a comparison that checks the value of a variable, which is set by an additional statement.

In short: if you remove an expression from a FILTER, it has to be the top-level expression, and then you have to remove the whole FILTER, otherwise the logic will be invalid.

@tobiasschweizer
Copy link
Contributor

In short: if you remove an expression from a FILTER, it has to be the top-level expression, and then you have to remove the whole FILTER, otherwise the logic will be invalid.

Ok, I understand! But how would that work for fulltext search? We allow users to use the match function, but internally we are creating a statement using the Lucene index.

@benjamingeer
Copy link

I'll add a check to make sure that match is at the top level of the FILTER, and throw an exception if it isn't.

@benjamingeer
Copy link

By the way, thank you for refactoring transformFilterPattern, it's so much easier to work on this way! :)

@tobiasschweizer
Copy link
Contributor

I'll add a check to make sure that match is at the top level of the FILTER, and throw an exception if it isn't.

Great!

By the way, thank you for refactoring transformFilterPattern, it's so much easier to work on this way! :)

Also great! I think your idea with the design doc for gravsearch is good, we should try to describe what is implemented in a more formalized way. And I think we will find a lot of inconsistencies.

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

No branches or pull requests

3 participants