Consistent handling of implicit and explicit LIMIT clauses
#1355
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RobinTF
commented
May 23, 2024
RobinTF
commented
May 23, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1355 +/- ##
==========================================
+ Coverage 88.77% 88.87% +0.10%
==========================================
Files 324 326 +2
Lines 28676 28926 +250
Branches 3172 3205 +33
==========================================
+ Hits 25456 25709 +253
+ Misses 2067 2065 -2
+ Partials 1153 1152 -1 ☔ View full report in Codecov by Sentry. |
RobinTF
commented
May 23, 2024
| mediaType == MediaType::qleverJson)) { | ||
| maxSend = MAX_NOF_ROWS_IN_RESULT; | ||
| } | ||
|
|
There was a problem hiding this comment.
Previously the exporter only applied maxSend to json results, tsv/csv results were not affected. For now I decided to keep this behaviour, because we can easily stream csv/tsv results without having to aggregate a giant string in memory, whereas we can't do that in JSON due to limitations of the JSON framework we use.
RobinTF
commented
May 23, 2024
joka921
reviewed
Jun 5, 2024
|
joka921
changed the title
LIMIT clauses.
joka921
changed the title
LIMIT clauses.LIMIT clauses
joka921
approved these changes
Jun 6, 2024
hannahbast
pushed a commit
that referenced
this pull request
Sep 8, 2024
#1355 changed the effect of the QLever-specific `send` parameter by clamping the `LIMIT` of the query accordingly, and adding one if none was there. This has the effect that, in the QLever UI, the result size of all queries with a result with more tha 100 rows was since shown as `100`. This PR tries to revert this unwanted effect while keeping the good stuff from #1355 (which unified the code for the various export formats and fixed various inconsistencies). The behavior is now as follows: 1. The query is first processed according to the `LIMIT` specified in the query. In particular, if no `LIMIT` is specified, the full result is materialized internally. 2. At most `send` bindings are exported. In the QLever JSON, there are now two fields: `resultsize` (the size of the internally computed result) and `sent` (the number of bindings exported). The idea is to add an option in the QLever UI for automatically adding a `LIMIT' to each query. That option should be activated by default (in order to save computation time), but a user should be able to deactivate it in case they are interested in the full result size. Future work: When one is only interested in the size of the full result, that can als be computed without fully materializing it. That would save RAM compared to the current approach.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There were some inconsistencies between explicit LIMIT clauses and the HTTP parameter
send(which is used by the QLever UI) which are fixed by this PR.Notable Changes after this PR is merged: