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

Parse all SPARQL Update Requests #1574

Merged
merged 13 commits into from
Oct 26, 2024

Conversation

Qup42
Copy link
Member

@Qup42 Qup42 commented Oct 22, 2024

The SPARQL parser now supports all kinds of UPDATEs. These are parsed into a unified class UpdateClause which is part of the ParsedQuery. This is another major step towards support for SPARQL UPDATE.

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 92.24806% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.04%. Comparing base (d46e69a) to head (185b522).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/parser/sparqlParser/SparqlQleverVisitor.cpp 92.74% 7 Missing and 2 partials ⚠️
src/parser/data/GraphRef.h 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1574      +/-   ##
==========================================
- Coverage   89.06%   89.04%   -0.02%     
==========================================
  Files         368      368              
  Lines       34039    34094      +55     
  Branches     3846     3847       +1     
==========================================
+ Hits        30316    30360      +44     
- Misses       2471     2481      +10     
- Partials     1252     1253       +1     

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

@Qup42 Qup42 marked this pull request as ready for review October 23, 2024 13:58
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.

A first pass over the code (not the tests)
This is already looking code, I only have minor comments and questions.
Keep up the good work!

src/parser/SparqlTriple.h Outdated Show resolved Hide resolved
src/parser/UpdateClause.h Outdated Show resolved Hide resolved
src/parser/data/GraphRef.h Show resolved Hide resolved
src/parser/UpdateClause.h Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.h Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
Comment on lines +476 to +488
auto checkTriples =
[&isVisibleIfVariable,
&ctx](const std::vector<SparqlTripleSimpleWithGraph>& triples) {
for (auto& triple : triples) {
if (!(isVisibleIfVariable(triple.s_) &&
isVisibleIfVariable(triple.p_) &&
isVisibleIfVariable(triple.o_))) {
reportError(ctx,
absl::StrCat("A triple contains a variable that was "
"not bound in the query body."));
}
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Is this duplicated/copy-pasted from some other type of query?
If so, remove that duplication by helper functions. Else I am going to check this some other time when I am less tired, this seems to be the most complicated rule.

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK this is not duplicated. But maybe a (currently private) of ParsedQuery can be used instead of isVisibleIfVariable.

});
std::ranges::move(visitVector(ctx->quadsNotTriples()),
std::back_inserter(triplesWithGraph));
return ad_utility::flatten(std::move(triplesWithGraph));
Copy link
Member

Choose a reason for hiding this comment

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

You are reordering the triples here. Is that correct, because the order has no semantics? Then write a short comment to make it clearer.

Copy link
Member Author

@Qup42 Qup42 Oct 25, 2024

Choose a reason for hiding this comment

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

To my understanding the order has no meaning here, no. In the end we get a set of quads that is added to/removed from one or multiple graphs. The "right" order can also not be reconstructed with the current setup. The quads <a> <b> <c> GRAPH <foo> { <d> <e> <f> } and GRAPH <foo> { <d> <e> <f> } . <a> <b> <c> can not be distinguished in their parsed form (meaning in the visitor).

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.

A few minor comments for the test design.
Consider merging in the current master branch to make the coverage report more consistent.

test/SparqlAntlrParserTestHelpers.h Outdated Show resolved Hide resolved
test/SparqlAntlrParserTest.cpp Outdated Show resolved Hide resolved
test/SparqlAntlrParserTest.cpp Outdated Show resolved Hide resolved
@sparql-conformance
Copy link

Copy link

sonarcloud bot commented Oct 25, 2024

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 you very much!

@joka921 joka921 merged commit 6e6aa7b into ad-freiburg:master Oct 26, 2024
22 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