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

Fix handling of EXISTS inside BIND #1794

Merged

Conversation

aucampia
Copy link
Member

@aucampia aucampia commented Apr 9, 2022

Summary of changes

This patch fixes an issue with BIND( EXISTS ... ) in SPARQL,
for example:

SELECT * WHERE {
    BIND(
        EXISTS {
            <http://example.com/a>
            <http://example.com/b>
            <http://example.com/c>
        }
        AS ?bound
    )
}

The graph pattern of EXISTS needs to be translated for it to operate
correctly during evaluation, but this was not happening. This patch
corrects that so that the graph pattern is translated as part of
translating BIND.

This patch also adds a bunch of tests for EXISTS to ensure there is no
regression and that various EXISTS cases function correctly.

Fixes #1472

Checklist

  • Checked that there aren't other open pull requests for
    the same change.
  • Added tests for any changes that have a runtime impact.
  • Checked that all tests and type checking passes.
  • For changes that have a potential impact on users of this project:
    • Considered updating our changelog (CHANGELOG.md).
  • Considered granting push permissions to the PR branch,
    so maintainers can fix minor issues and keep your PR up to date.

@aucampia
Copy link
Member Author

aucampia commented Apr 9, 2022

@GreenfishK I think you know this stuff better than most, if you have time for a review it will be appreciated.

@aucampia
Copy link
Member Author

aucampia commented Apr 9, 2022

@niklasl @rchateauneu you may also have some insights here.

@aucampia aucampia added review wanted This indicates that the PR is ready for review fix Fixes an issue labels Apr 9, 2022
@aucampia aucampia requested a review from edmondchuc April 9, 2022 14:42
@GreenfishK
Copy link

@GreenfishK I think you know this stuff better than most, if you have time for a review it will be appreciated.

Hi @aucampia, I just saw your comment as I was not online over the weekend. Is the review still required?

@aucampia
Copy link
Member Author

@GreenfishK I think you know this stuff better than most, if you have time for a review it will be appreciated.

Hi @aucampia, I just saw your comment as I was not online over the weekend. Is the review still required?

If you have time yes please, I'm fairly confident about this change but I'm not really as familiar with the SPARQL parsing, translation and evaluation as I think you are. Further if you can think of more corner cases to test, or just tests that would make you more comfortable with this change I would be happy to add them also.

@aucampia aucampia force-pushed the iwana-20220329T2225-sparql_exists_as branch from 637eb7b to f1ab6b9 Compare April 13, 2022 20:06
@aucampia aucampia requested a review from a user April 13, 2022 20:06
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Did you intend to include the change to tox.ini? Not that I have any objection at all, just checking.

@aucampia
Copy link
Member Author

aucampia commented Apr 14, 2022

Did you intend to include the change to tox.ini? Not that I have any objection at all, just checking.

I did intend to include it, though it probably would have been better to not include it. It is mainly in the PR because the previous log format did not include the function name and it was a bit hard to read, and to fix this issue it helped a lot to see the function name that things were coming from.

Thanks for the reviews.

This patch fixes an issue with `BIND( EXISTS ... )` in SPARQL,
for example:

```sparql
SELECT * WHERE {
    BIND(
	EXISTS {
	    <http://example.com/a>
	    <http://example.com/b>
	    <http://example.com/c>
	}
	AS ?bound
    )
}
```

The graph pattern of `EXISTS` needs to be translated for it to operate
correctly during evaluation, but this was not happening. This patch
corrects that so that the graph pattern is translated as part of
translating `BIND`.

This patch also adds a bunch of tests for EXISTS to ensure there is no
regression and that various EXISTS cases function correctly.

Fixes RDFLib#1472
@aucampia aucampia force-pushed the iwana-20220329T2225-sparql_exists_as branch from f1ab6b9 to ff49579 Compare April 15, 2022 15:26
@aucampia
Copy link
Member Author

Going to merge this on 2022-04-17 if there is no further feedback.

Copy link
Contributor

@edmondchuc edmondchuc left a comment

Choose a reason for hiding this comment

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

Tests look good to me.

@aucampia
Copy link
Member Author

Won't merge before #1838

@aucampia aucampia merged commit 65cae4e into RDFLib:master Apr 18, 2022
@aucampia aucampia deleted the iwana-20220329T2225-sparql_exists_as branch June 4, 2022 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Fixes an issue review wanted This indicates that the PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using the result of a SPARQL query with EXISTS and AS raises an exception
3 participants