-
Notifications
You must be signed in to change notification settings - Fork 56
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
Implement a function that executes an UPDATE request #1607
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small suggestions for improvements + now that we have the #1604 in the master, please merge it in to make the tests work.
auto expectExecuteUpdate = | ||
[&executeUpdate]( | ||
const std::string& update, | ||
const testing::Matcher<const DeltaTriples&>& deltaTriplesMatcher) { | ||
EXPECT_THAT(executeUpdate(update), deltaTriplesMatcher); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could just return a matcher (return ResultOf(executeUpdate, deltaTriplesMatcher);
),
The you have the EXPECT_THAT
with the single calls, and the failure messages have the correct line numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not super important though)
test/ExecuteUpdateTest.cpp
Outdated
const testing::Matcher<const std::vector<::IdTriple<>>&>& | ||
toInsertMatcher, | ||
const testing::Matcher<const std::vector<::IdTriple<>>&>& | ||
toDeleteMatcher) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above, you can also add the source_location
to the matching functions and use generateLocationTrace
to get better messages on failure.
Conformance check passed ✅No test result changes. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1607 +/- ##
==========================================
+ Coverage 89.17% 89.18% +0.01%
==========================================
Files 372 372
Lines 34579 34627 +48
Branches 3912 3915 +3
==========================================
+ Hits 30835 30882 +47
- Misses 2471 2472 +1
Partials 1273 1273 ☔ View full report in Codecov by Sentry. |
Quality Gate passedIssues Measures |
Since #1355, the query `LIMIT` is clamped to the value of the QLever-specific `send` parameter. In particular, this broke the display of the result size in the QLever UI, which sets `send=100` when showing the first page of results for a query. This change restores the old behavior for the `send` parameter, yet makes good use of the new lazy query processing. Specifically, lazily computed results blocks are now processed as follows: (1) the first result blocks before OFFSET are computed but skipped; (2) then results are computed and materialized until the value of the `send` parameter is reached; (3) then results are computed and counted but *not* materialized until the LIMIT is reached; (3) all remaining blocks are not even computed. The QLever JSON now has two new fields `resultSizeTotal` and `resultSizeExported`, with the corresponding values. For the sake of backwards-compatibility, the old `resultsize` field is still there and has the same value as the `resultSizeTotal` field. For the same reason, the `send` parameter keeps its name for now, but should be renamed to `exportLimit` eventually. On the side fixed, dropped the hard limit of `MAX_NOF_ROWS_IN_RESULT = 1'000'000` for JSON results. Also fix the compilation error introduced by the interplay of the merge of #1603 and #1607 . Fixes #1605 and #1455 .
This is another step towards support for `SPARQL UPDATE`. The function takes a `ParsedQuery` that contains a graph update (and UPDATE request that inserts and/or deletes triples that are computed from a WHERE clause to/from a given graph), executes the query, and passes the result to a `DeltaTriples` object.
Since ad-freiburg#1355, the query `LIMIT` is clamped to the value of the QLever-specific `send` parameter. In particular, this broke the display of the result size in the QLever UI, which sets `send=100` when showing the first page of results for a query. This change restores the old behavior for the `send` parameter, yet makes good use of the new lazy query processing. Specifically, lazily computed results blocks are now processed as follows: (1) the first result blocks before OFFSET are computed but skipped; (2) then results are computed and materialized until the value of the `send` parameter is reached; (3) then results are computed and counted but *not* materialized until the LIMIT is reached; (3) all remaining blocks are not even computed. The QLever JSON now has two new fields `resultSizeTotal` and `resultSizeExported`, with the corresponding values. For the sake of backwards-compatibility, the old `resultsize` field is still there and has the same value as the `resultSizeTotal` field. For the same reason, the `send` parameter keeps its name for now, but should be renamed to `exportLimit` eventually. On the side fixed, dropped the hard limit of `MAX_NOF_ROWS_IN_RESULT = 1'000'000` for JSON results. Also fix the compilation error introduced by the interplay of the merge of ad-freiburg#1603 and ad-freiburg#1607 . Fixes ad-freiburg#1605 and ad-freiburg#1455 .
This is another step towards support for
SPARQL UPDATE
. The function takes aParsedQuery
that contains a graph update (and UPDATE request that inserts and/or deletes triples that are computed from a WHERE clause to/from a given graph), executes the query, and passes the result to aDeltaTriples
object.