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

Support BASE declarations in SPARQL queries #1786

Merged
merged 13 commits into from
Feb 14, 2025

Conversation

RobinTF
Copy link
Collaborator

@RobinTF RobinTF commented Feb 10, 2025

So far, the BASE keyword was supported for Turtle inputs, but not as part of a SPARQL query. Now it is correctly supported in all cases

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.

Several comments about details

src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
Comment on lines 1227 to 1235
for (auto* child : ctx->children) {
if (auto* baseDecl = dynamic_cast<Parser::BaseDeclContext*>(child)) {
visit(baseDecl);
} else {
auto* prefixDecl = dynamic_cast<Parser::PrefixDeclContext*>(child);
AD_CORRECTNESS_CHECK(prefixDecl);
visit(prefixDecl);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

What is wrong with the simpler older version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Imagine the following case:

BASE <http://example.com>
PREFIX test: <test>
BASE <http://other.example.com>
PREFIX test2: <test>
BASE <http://alternative.example.com>
SELECT * FROM {
  <s> ?p test:abc
}

Then the first BASE should apply to the first prefix, and the second BASE should apply to the second prefix, and the third BASE should apply to everything else that follows. So to ensure that we apply BASE to PREFIX correctly, we can't parse them after each other, and instead have to interleave parsing.

src/parser/sparqlParser/SparqlQleverVisitor.cpp Outdated Show resolved Hide resolved
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.05%. Comparing base (1570033) to head (4ba7fcb).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1786      +/-   ##
==========================================
+ Coverage   90.02%   90.05%   +0.03%     
==========================================
  Files         396      396              
  Lines       37974    38010      +36     
  Branches     4262     4267       +5     
==========================================
+ Hits        34185    34230      +45     
+ Misses       2493     2489       -4     
+ Partials     1296     1291       -5     

☔ 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.

A small round of things, but we can almost merge this.

src/engine/sparqlExpressions/StringExpressions.cpp Outdated Show resolved Hide resolved
src/engine/sparqlExpressions/StringExpressions.cpp Outdated Show resolved Hide resolved
src/engine/sparqlExpressions/StringExpressions.cpp Outdated Show resolved Hide resolved
src/parser/sparqlParser/SparqlQleverVisitor.h Outdated Show resolved Hide resolved
@sparql-conformance
Copy link

Conformance check passed ✅

Test Status Changes 📊

Number of Tests Previous Status Current Status
1 Failed Passed
1 Failed Intended

Details: https://qlever.cs.uni-freiburg.de/sparql-conformance-ui?cur=4ba7fcbac73a7462973ce710ca07d934b8246bcc&prev=1570033d07eb625dd3c2624c866eeb241f8639ef

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 changed the title Add support for BASE declaration Support BASE declarations in SPARQL queries Feb 14, 2025
@joka921 joka921 merged commit 6349abd into ad-freiburg:master Feb 14, 2025
23 of 24 checks passed
@RobinTF RobinTF deleted the base-support branch February 14, 2025 22:54
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