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

Add datasets from parameters for Query and Update #1714

Merged
merged 10 commits into from
Jan 27, 2025

Conversation

Qup42
Copy link
Member

@Qup42 Qup42 commented Jan 17, 2025

The default graphs and named graphs for a query or update can now also be specified as parameters to the HTTP GET/POST request (in addition to specifying them via FROM [NAMED] clauses, which was also supported previously.
In particular, the parameters default-graph-uri and named-graph-uri are now supported by queries, and the parameters using-graph-uri and using-named-graph-uri are supported by updates. Note that these parameters are part of the standardized SPARQL protocol.
Fixes #1712

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 55.26316% with 34 lines in your changes missing coverage. Please review.

Project coverage is 89.92%. Comparing base (ff47922) to head (8fab30d).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/engine/Server.cpp 51.42% 34 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1714   +/-   ##
=======================================
  Coverage   89.92%   89.92%           
=======================================
  Files         393      393           
  Lines       37614    37626   +12     
  Branches     4231     4231           
=======================================
+ Hits        33823    33837   +14     
+ Misses       2494     2491    -3     
- Partials     1297     1298    +1     

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

Thank you very much, I have some suggestions for cleaner code etc.

src/engine/Server.cpp Outdated Show resolved Hide resolved
Comment on lines 1001 to 1002
} else {
AD_FAIL();
Copy link
Member

Choose a reason for hiding this comment

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

Can this statically happen?
Otherwise you can replace the else if constexpr by else static_assert

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean by doing this statically? The comment is now applied and also a static_assert(std::is_same_v<Operation, Query> || std::is_same_v<Operation, Update>) at the beginning (which my IDE marked at some time). What would be a good style for doing this (the static_assert in the else is redundant now)?

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind the redundant static assert in the beginning.
The one in the beginning gives nice documentation + error messages,
And in the if-else it is useful to prevent bugs etc. So I really like the way it is now.

Comment on lines 1046 to 1049
return operation.update_;
} else {
AD_FAIL();
}
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above.

@Qup42
Copy link
Member Author

Qup42 commented Jan 22, 2025

There was one random test failure in a run that was most likely not from this PR: https://github.com/ad-freiburg/qlever/actions/runs/12833696568/job/35789345340?pr=1714

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.

Two and a half very minor comments, this is close to finished now.

src/engine/Server.cpp Outdated Show resolved Hide resolved
src/engine/Server.cpp Outdated Show resolved Hide resolved
Comment on lines 1001 to 1002
} else {
AD_FAIL();
Copy link
Member

Choose a reason for hiding this comment

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

I don't mind the redundant static assert in the beginning.
The one in the beginning gives nice documentation + error messages,
And in the if-else it is useful to prevent bugs etc. So I really like the way it is now.

Qup42 added 2 commits January 24, 2025 11:11
# Conflicts:
#	src/engine/Server.cpp
#	src/engine/Server.h
@sparql-conformance
Copy link

@joka921 joka921 merged commit 50e5108 into ad-freiburg:master Jan 27, 2025
23 of 24 checks passed
@Qup42 Qup42 deleted the addAllDatasetFromQuery branch January 27, 2025 18:11
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.

Not supported: default-graph-uri parameter
2 participants