Implement CONSTRUCT query processing #528
Conversation
RobinTF
force-pushed
the
construct-query-parser
branch
from
9bf0563
to
f96bdff
Compare
src/engine/Server.cpp
Outdated
| const auto throwIfConstructClause = [&pq]() { | ||
| if (pq.hasConstructClause()) { | ||
| throw std::runtime_error{ | ||
| "CONSTRUCT queries only support turtle syntax right now"}; |
There was a problem hiding this comment.
Two remarks remaining here:
- You should add the turtle media type in the
supportedMediaTypesabove (line 345) s.t. we can also specify it via an accept header. - replace "turtle syntax" by "RDF Turtle as an export format". And Add "Please specify "action=turtle-export" as a query parameter or the accept header "...(add corresponding header here)
There was a problem hiding this comment.
It's currently called turtle_export to be consistent with csv_export and tsv_export and all the other options
| AD_CHECK(_name.length() > 1); | ||
| // normalise notation for consistency | ||
| if (_name[0] == '$') { | ||
| _name[0] = '?'; |
There was a problem hiding this comment.
if you don't check that the first character is either $ or ? you can do the
overwrite unconditionally. But also asserting this would probably be better.
There was a problem hiding this comment.
I think it's better to avoid the unecessary write operation if possible and let branch prediction skip this constructor in most cases
| @@ -65,5 +74,8 @@ class Variable { | |||
| } | |||
|
|
|||
| // ___________________________________________________________________________ | |||
| [[nodiscard]] std::string toString() const { return _name; } | |||
| [[nodiscard]] std::string toSparql() const { return _name; } | |||
There was a problem hiding this comment.
This can also be a const string& ,
If other types return std::string here, no harm is done imho.
There was a problem hiding this comment.
This code is supposed to be removed sooner or later, so I'd prefer to keep it as-is for now, even if this might avoid an unecessary copy
|
Thanks a lot, @RobinTF and @joka921, for this PR! I just tried it with https://qlever.cs.uni-freiburg.de/olympics and noted three things:
|
I didn't have to rebuild any index for this. And none of the code was touched by this PR (or at least it shouldn't have touched any parts)
That's odd. I assumed it would just not print the PREFIX declarations in the turtle format, but I will have a look into this when I start writing tests for all of this code. The diff was getting fairly large so @joka921 and I decided it would be best to break this up into several PRs, to not conflict with #529
A PR that supports different formats like tsv/csv/json is in the making. However, the SPARQL specification is pretty vague about serialization with non-turtle formats so we might need to come up with some qlever-specific rules to make those formats work well enough for this. |
|
@RobinTF Thanks for the feedback, Robin! @1: I just tried it with another knowledge graph and it worked without having to rebuild the index. So the index I tried first was probably old and would have needed rebuilding already for the previous version of the master. @2: Good idea to break this up. The problem I described is probably just a simple bug. It seems that when the CONSTRUCT clause is evaluated, the prefix map either has not yet been parsed or is not available for some other reason. @3: More formats are of course great, but I am happy with text/turtle for the moment. My comment was that without specifying an Accept header, the export currently fails. For example, the following command gives the error "CONSTRUCT queries only support RDF Turtle as an export format right now". The reason is that some other media type (probably application/qlever-results+json) is chosen by default. My point was that for a CONSTRUCT query, the default media type (if none other is specified) should be text/turtle .
|
This PR starts implementing support for CONSTRUCT queries. Work in progress.