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

implement functions Iri() / Uri() #1401

Merged
merged 9 commits into from
Jul 22, 2024

Conversation

realHannes
Copy link
Collaborator

No description provided.

Hannah Bast and others added 4 commits July 6, 2024 05:16
This is a first draft. It does something meaningful already, but is not
quite standard-conform yet.

Also, I am not sure whether the `StringValueGetter` is the right way to
go here, since the argument can also be an `Iri`, in which case the
function should be the identity.
@realHannes realHannes requested a review from joka921 July 15, 2024 12:44
Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

Thank your, there are some implementation details that I also have to consider, maybe we both can do some research until wednesday.

IdOrLiteralOrIri IriOrUriValueGetter::operator()(
const LiteralOrIri& litOrIri,
[[maybe_unused]] const EvaluationContext* context) const {
const auto& optIri = Iri::fromIrirefWithBasicUrlCheck(
Copy link
Member

Choose a reason for hiding this comment

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

I think for now you should simply return the content without any normalization.
The whole resolving against base IRI stuff is something that QLever currently doesn't support at all, so we should implement it in one go at a later time.
In particlar QLever currently supports IRIs that are not valid IRIs like <is-a> etc.

The only thing that is worth considering (check what happens there), is if a string contains special characters that are not allowed in IRIs. (which are those, should we normalize them), but there I also would have to check on how to properly normalize or escape them (percent encoding? anything else?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah i see, i didn't realize that BASE was a declaration similar to a PREFIX at the beginning of a query.

Given that we implement IRI() and URI() in combination, and IRIs allow a wider range of characters, if we normalize at all, probably only w.r.t. the IRI range.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BASE is already defined by the parser grammar, but not implemented yet. Resolving against BASE could be maybe done with help of sparqlExpression::EvaluationContext.

src/parser/Iri.cpp Outdated Show resolved Hide resolved
src/parser/Iri.h Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jul 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.27%. Comparing base (6a6a4b8) to head (cb1fdbd).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1401      +/-   ##
==========================================
+ Coverage   89.14%   89.27%   +0.13%     
==========================================
  Files         332      336       +4     
  Lines       29498    29754     +256     
  Branches     3291     3315      +24     
==========================================
+ Hits        26296    26564     +268     
+ Misses       2050     2027      -23     
- Partials     1152     1163      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

The test cases don't yet quite cover the relevant cases, but as you already have the infrastructure, this should be fixable.

Comment on lines 643 to 648
"<https://example.org/bim%20A%00c%03doof%11c%12testescape/%13test>";
std::string objectQuery =
"SELECT ?o WHERE { "
"BIND(IRI(<https://example.org/bim%20A%00c%03doof%11c%12testescape/"
"%13test>) AS ?o) }";
std::string expectedXml = makeXMLHeader({"o"}) +
Copy link
Member

Choose a reason for hiding this comment

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

Those are not the escapes I meant, those change the meaning of the IRI.
I meant stuff like <https:\\u0020example.org>. etc.
Also the IRI function in the bind can take a literal like IRI("example\\n.org").

Let me know if you need help.

Copy link
Member

Choose a reason for hiding this comment

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

I see that you implemented a separate test for the strings, but the first point remains valid.

Copy link

@joka921 joka921 merged commit 6e26c00 into ad-freiburg:master Jul 22, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants