Add a VALUES clause to the query of a SERVICE clause to simplify the execution #1341

UNEXENU · 2024-05-08T13:40:34Z

If the query tree, with which the result of a SERVICE clause is joined, leads to a small result (The definition of "small" can be set by a runtime parameter, the current default is 100), then this result is added to the query that is sent to the endpoint of the SERVICE, making it much easier to process, and the result of the SERVICE query potentially much smaller.

joka921

A first round of reviews.

joka921 · 2024-05-10T08:06:01Z

src/engine/Service.cpp

@@ -92,6 +95,44 @@ ResultTable Service::computeResult() {
  serviceIriString.remove_suffix(1);
  ad_utility::httpUtils::Url serviceUrl{serviceIriString};

+  if (siblingTree_ != nullptr) {


This should be in its own function that returns e.g. an optional<string> that returns the VALUES clause or nothing.

joka921 · 2024-05-10T08:06:32Z

src/engine/Service.cpp

+    // reduce complexity of the SERVICE query.
+    auto siblingResult = siblingTree_->getResult();
+
+    const size_t rowLimit = 100;


Make this configurable for experiments by registering it in the RuntimeParameters class.

joka921 · 2024-05-10T08:07:40Z

src/engine/Service.cpp

+    auto siblingResult = siblingTree_->getResult();
+
+    const size_t rowLimit = 100;
+    if (siblingResult->size() < rowLimit) {


If you do it the other way round (if size >= limit => immediately return/break), then the code gets less nested.

joka921 · 2024-05-10T08:12:29Z

src/engine/Service.cpp

+        if (it == siblingVariables.end()) {
+          continue;
+        }
+        const auto& sVar = *it;


The semantics of this are currently a bit wrong for multiple variables:

If I have the following table:
?x ?y ?z

Then I need the values clause

VALUES (?x ?y ?z) { (<x1> <y1> <z1>) (<x2> <y2> <z2>)}
with multiple variables , not three individual values clauses, otherwise I get

8 results (the cartesian product) instead of the 2 lines).

(For the exact syntax of multi varaible clauses you can look around the standard or play with the UI, the above might contain typos).

joka921 · 2024-05-10T08:15:01Z

src/engine/Service.cpp

+      std::string valueClauses = "{ ";
+      for (const auto& lVar : parsedServiceClause_.visibleVariables_) {
+        auto it = siblingVariables.find(lVar);
+        if (it == siblingVariables.end()) {


First in a separate step compute the relevant column indices for the variables, and then simply iterate over those.
That makes the large loop more readable and possibly also more efficient.

joka921 · 2024-05-10T08:16:26Z

src/engine/Service.cpp

+          const auto& optionalString =
+              ExportQueryExecutionTrees::idToStringAndType(
+                  siblingTree_->getRootOperation()->getIndex(),
+                  siblingResult->idTable()(rowIndex, sVar.second.columnIndex_),
+                  siblingResult->localVocab());


This loop can also be refactored for readability, but we can do this once the other issues are resolved.

joka921 · 2024-05-10T08:22:20Z

test/ServiceTest.cpp

+  std::string_view expectedSparqlQuery5 =
+      "PREFIX doof: <http://doof.org> SELECT ?x ?y "
+      "WHERE { VALUES ?x { <x> <blu> } . VALUES ?y { <y> <bla> } . "
+      "?x <ble> ?y . }";


As already stated, this is not quite right (or as efficient as it can be ) yet, but we will get there (see the comment above)

codecov · 2024-05-10T08:30:04Z

Codecov Report

Attention: Patch coverage is 98.90110% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 88.81%. Comparing base (f9c3132) to head (21b42be).

Files	Patch %	Lines
src/engine/Service.cpp	98.30%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1341      +/-   ##
==========================================
+ Coverage   88.77%   88.81%   +0.04%     
==========================================
  Files         324      324              
  Lines       28676    28762      +86     
  Branches     3172     3187      +15     
==========================================
+ Hits        25456    25544      +88     
  Misses       2067     2067              
+ Partials     1153     1151       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

UNEXENU · 2024-05-16T08:40:36Z

src/engine/Service.cpp

+    for (size_t i = 0; i < commonColumnIndices.size(); ++i) {
+      const auto& optionalString = ExportQueryExecutionTrees::idToStringAndType(
+          siblingTree_->getRootOperation()->getIndex(),
+          siblingResult->idTable()(rowIndex, commonColumnIndices[i]),
+          siblingResult->localVocab());


Somehow the localVocab accessed here with siblingResult->localVocab() doesn't contain useful data.
As a result the optionalString has no Content.

I'll have a look at this. I however don't suspect that the problem is the local vocab.

Can you send me a reproducer (Dataset + Query) where something doesn't work as expected?
I just tried a simple example with one variable where this worked.

Ok sry, here is a reproducable example query, i use it with Olympics Dataset but it doesn't use the local dataset anyway:

PREFIX schema: <http://schema.org/> PREFIX imdb: <https://www.imdb.com/> PREFIX rdfs: <http://www.w3.org/2000/01/rdf-schema#> PREFIX p: <http://www.wikidata.org/prop/> SELECT ?imdb_id ?imdb_votes ?imdb_rating WHERE { VALUES ?imdb_id { "tt0477348" "tt0118715" "tt0116282" } . SERVICE <https://qlever.cs.uni-freiburg.de/api/imdb> { ?movie_imdb imdb:id ?imdb_id . ?movie_imdb imdb:type "movie" . ?movie_imdb imdb:numVotes ?imdb_votes . ?movie_imdb imdb:averageRating ?imdb_rating . } } ORDER BY DESC(?imdb_votes)

Expected result: 3 rows with the given imdb_id values.
Actual result: 0 rows, Debug log shows that the sent Service-Query contains a Values Clause with imdb_id as key but no values.

I figured it out....
Your code is absolutely working as it should, the problem was the implementation of the VALUES clause.
Basically you could neither evaluate it twice, nor read it from the cache during the same query.
I have pushed a quick fix to your branch (just do a git pull in your local copy to get it) and everything should work as expected.

joka921

A first round of reviews.

I managed to get a simple query working with this, So we are on a good track with this.

joka921 · 2024-05-16T08:52:04Z

src/engine/QueryPlanner.cpp

+  auto bRootOp = std::dynamic_pointer_cast<Service>(b._qet->getRootOperation());
+
+  // Exactly one of the two Operations can be a service.
+  if (aRootOp == bRootOp) {


Suggested change

if (aRootOp == bRootOp) {

if (static_cast<bool>(aRootOp) == static_cast<bool>(bRootOp)) {

The other one is very fishy (expects that you don't get passed the same Service twice).

src/engine/QueryPlanner.cpp

joka921 · 2024-05-16T08:58:46Z

src/engine/Service.cpp

+  if (auto valuesClause = getSiblingValuesClause(); valuesClause.has_value()) {
+    parsedServiceClause_.graphPatternAsString_ =
+        "{ " + valuesClause.value() +
+        parsedServiceClause_.graphPatternAsString_.substr(2);


You should probably find the first occurence of the {, I am not sure if this breaks as soon as someone puts random spaces in their query.

src/engine/Service.cpp

joka921 · 2024-05-16T09:04:10Z

src/engine/Service.h

@@ -60,6 +60,10 @@ class Service : public Operation {
          GetTsvFunction getTsvFunction = sendHttpOrHttpsRequest,
          std::shared_ptr<QueryExecutionTree> siblingTree = nullptr);

+  inline void setSiblingTree(std::shared_ptr<QueryExecutionTree> siblingTree) {


The inline doesn't do anything here.
And add a short docstring, the semantics of a sibling tree should be documented.

src/engine/Service.h

joka921 · 2024-05-16T09:07:05Z

src/engine/Service.cpp

+    for (size_t i = 0; i < commonColumnIndices.size(); ++i) {
+      const auto& optionalString = ExportQueryExecutionTrees::idToStringAndType(
+          siblingTree_->getRootOperation()->getIndex(),
+          siblingResult->idTable()(rowIndex, commonColumnIndices[i]),
+          siblingResult->localVocab());


I'll have a look at this. I however don't suspect that the problem is the local vocab.

joka921 · 2024-05-16T09:08:13Z

src/engine/Service.cpp

+  if (commonColumnIndices.size() > 1) {
+    vars = "(" + vars + ")";
+  }


How about you ALWAYS add the parentheses, they also work for only one variable, and this makes the code simpler.

joka921 · 2024-05-16T09:29:09Z

src/engine/Service.cpp

+    for (size_t i = 0; i < commonColumnIndices.size(); ++i) {
+      const auto& optionalString = ExportQueryExecutionTrees::idToStringAndType(
+          siblingTree_->getRootOperation()->getIndex(),
+          siblingResult->idTable()(rowIndex, commonColumnIndices[i]),
+          siblingResult->localVocab());


Can you send me a reproducer (Dataset + Query) where something doesn't work as expected?
I just tried a simple example with one variable where this worked.

joka921 · 2024-05-16T09:30:23Z

src/engine/Service.cpp

+
+  std::vector<ColumnIndex> commonColumnIndices;
+  const auto& siblingVars = siblingTree_->getVariableColumns();
+  std::string vars = "";


There is one bug remaining:
If a row (combination of all relevant variables) appears multiple times in the sibling result, then you may only store it once in the VALUES clause, otherwise the SERVICE will duplicate each result leading to wrong results with too many rows.

UNEXENU · 2024-05-17T09:20:09Z

Theres a bug (introduced by this branch) in using the cache for the Service Operation with the following Query:

PREFIX schema: <http://schema.org/>
PREFIX imdb: <https://www.imdb.com/>
PREFIX rdfs: <http://www.w3.org/2000/01/rdf-schema#>
PREFIX p: <http://www.wikidata.org/prop/>
SELECT ?imdb_id ?imdb_votes ?imdb_rating WHERE {
VALUES (?imdb_id) { ("tt0477348") ("tt0118715") } .
  SERVICE <https://qlever.cs.uni-freiburg.de/api/imdb> {
  VALUES ?imdb_id { "tt0477348" "tt0118715" "tt0116282" } .
	?movie_imdb imdb:id ?imdb_id .
	?movie_imdb imdb:type "movie" .
    ?movie_imdb imdb:numVotes ?imdb_votes .
    ?movie_imdb imdb:averageRating ?imdb_rating .
  }
}
ORDER BY DESC(?imdb_votes)

Steps to reproduce: Run the query once. Then comment the first value clause out such that only the service clause remains, and run again.

Expected behavior: First run: 2 rows. Second Run: 3 rows

Actual behavior: First run: 2 rows. Second Run: 2 rows (Service result cached)

Conclusion: The Service Operations result will be loaded from Cache despite the change in its query issued by not injecting (or modifying) the siblings Result values.

joka921 · 2024-05-17T11:49:01Z

Theres a bug (introduced by this branch) in using the cache for the Service Operation with the following Query:
PREFIX schema: <http://schema.org/>
PREFIX imdb: <https://www.imdb.com/>
PREFIX rdfs: <http://www.w3.org/2000/01/rdf-schema#>
PREFIX p: <http://www.wikidata.org/prop/>
SELECT ?imdb_id ?imdb_votes ?imdb_rating WHERE {
VALUES (?imdb_id) { ("tt0477348") ("tt0118715") } .
  SERVICE <https://qlever.cs.uni-freiburg.de/api/imdb> {
  VALUES ?imdb_id { "tt0477348" "tt0118715" "tt0116282" } .
	?movie_imdb imdb:id ?imdb_id .
	?movie_imdb imdb:type "movie" .
    ?movie_imdb imdb:numVotes ?imdb_votes .
    ?movie_imdb imdb:averageRating ?imdb_rating .
  }
}
ORDER BY DESC(?imdb_votes)
Steps to reproduce: Run the query once. Then comment the first value clause out such that only the service clause remains, and run again.

Expected behavior: First run: 2 rows. Second Run: 3 rows

Actual behavior: First run: 2 rows. Second Run: 2 rows (Service result cached)

Conclusion: The Service Operations result will be loaded from Cache despite the change in its query issued by not injecting (or modifying) the siblings Result values.

@UNEXENU Thank you for analyzing this... The fix for this is easy: Change the getCacheKeyImpl of the ServiceClasss.t. it also contains the cacheKey of the sibling tree (if it exists).

joka921

This is already very useful and currently running on the Wikidata instance.
To merge this first step, there is still missing:

Unit tests (see the codecov tool and my suggestions in the review).
The fix ( + unit test) of the caching bug (sibling tree has to be part of cache key).
During the building of the Values clause you should regularly check the cancellation token (throwIfCancelled, see in the remaining code).

Let me know if there's things that I can assist you with.

joka921 · 2024-05-21T10:22:14Z

src/engine/QueryPlanner.cpp

+  SubtreePlan plan =
+      jcs.size() == 1
+          ? makeSubtreePlan<Join>(qec, a._qet, b._qet, jcs[0][0], jcs[0][1])
+          : makeSubtreePlan<MultiColumnJoin>(qec, a._qet, b._qet);


You should add unit tests for this planning (in the QueryPlannerTest.cpp and QueryPlannerTestHelpers.h you can add a matcher for a service clause, have a look at that code, and let me know if there is trouble)

I have implemented a simple test for this in this commit, let me know if that is sufficient or provide feedback for further improvements.

joka921 · 2024-05-21T10:28:18Z

src/engine/Service.cpp

    }
+
+    rowSet.insert(row);
    values += row + " ";


For all your someString += something + something other
use absl::StrAppend. It is more efficient and more idiomatic. (three places in this file)

joka921

Two small changes,
But we are almost ready to merge.
Sorry for the delay, I was on vacation.

joka921 · 2024-05-28T12:40:16Z

src/engine/Service.cpp

  ad_utility::HashSet<std::string> rowSet;

  std::string values = " { ";
  for (size_t rowIndex = 0; rowIndex < siblingResult->size(); ++rowIndex) {
    std::string row = "(";
-    for (size_t i = 0; i < commonColumnIndices.size(); ++i) {
+    for (const auto& columnIdx : commonColumnIndices) {


Inide the for each row loop (the outer one) should probably be one cancellation check per iterationl

joka921 · 2024-05-28T12:41:15Z

test/QueryPlannerTestHelpers.h

@@ -283,6 +284,9 @@ constexpr auto OrderBy = [](const ::OrderBy::SortedVariables& sortedVariables,
 // Match a `UNION` operation.
 constexpr auto Union = MatchTypeAndOrderedChildren<::Union>;

+// Match a `SERVICE` operation.
+constexpr auto Service = MatchTypeAndOrderedChildren<::Service>;


Please implement the matcher s.t. it also takes a matcher for the sibling tree and the graphPatternAsString
etc.

sonarqubecloud · 2024-05-31T16:56:13Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

There is a bug in the optimization introduced by #1341, which checks whether the sibling operand of a join with a `SERVICE` operations has a small result (and if yes, passes that result to the `SERVICE` clause as `VALUES` in order to reduce the result size of the `SERVICE` call). Instead of just adding the sibling operand to the existing `Service` operation (which led to an object that should have been `const` being changed), now a new `Service` operation that includes the sibling is created. In particular, this fixes https://qlever.cs.uni-freiburg.de/osm-planet, which worked before #1341 and now works again as it should.

With this commit, the optimization that constrains a `SERVICE` query using a `VALUES` clause when the sibling of the `SERVICE` is small (originally introduced in #1341) is also applied for `OPTIONAL` and `MINUS` clauses when the right operand is a `SERVICE`.

Previously, the sibling of a `SERVICE` (see #1341 for details) was computed twice. This was not a problem for small siblings result, because they were cached, making the second computation actually a cheap cache read. It was however a problem if the result was too large for the cache. It then was computed twice, although only one of the computations was actually used, because the SERVICE discarded the result because it was too large. This commit fixes this behavior. The sibling is now only computed once, and only gets explicitly shared with the `SERVICE` if it is sufficiently small.

Adds the commandline argument `service-max-value-rows` allowing to set the maximum number of rows passed to a `Service` operation (see #1341 and #1502 for details). The default value is set to 10,000 rows.

UNEXENU added 2 commits May 8, 2024 14:43

Added Sibling Result integration for Service Operation + test

b927302

fixed formatting

4780aea

joka921 requested changes May 10, 2024

View reviewed changes

UNEXENU added 2 commits May 13, 2024 11:58

Fixed multi-value clause creation and related things

ec083e7

Attempt to create ServiceJoin in QueryPlanner. Doesn't work yet

7d90398

UNEXENU commented May 16, 2024

View reviewed changes

joka921 requested changes May 16, 2024

View reviewed changes

joka921 and others added 2 commits May 16, 2024 17:23

A small fix in the VALUES clause...

28946b9

Fixed duplications in values Clause, MultiColumnJoin

aeb638e

joka921 requested changes May 21, 2024

View reviewed changes

Fixed caching bug + unit tests

2afa979

joka921 requested changes May 28, 2024

View reviewed changes

UNEXENU and others added 6 commits May 31, 2024 12:17

Improved tests, added checkCancellation

94ad347

small changes

051bef2

Merge branch 'refs/heads/master' into ServiceImprovements

4673248

merge in the master.

fbb6e0b

Improve the test coverage etc.

50525b1

Actually fix the test

21b42be

joka921 approved these changes Jun 5, 2024

View reviewed changes

joka921 changed the title ~~Service Improvements~~ Add a VALUES clause to the query of a SERVICE clause to simplify the execution Jun 5, 2024

joka921 merged commit bb6e2f9 into ad-freiburg:master Jun 5, 2024
19 checks passed

hannahbast mentioned this pull request Aug 1, 2024

Fix query planning for join with SERVICE clause #1422

Merged

UNEXENU mentioned this pull request Sep 17, 2024

Apply SERVICE optimization for OPTIONAL and MINUS #1502

Merged

UNEXENU mentioned this pull request Oct 15, 2024

Only compute the sibling of a SERVICE once #1556

Merged

UNEXENU mentioned this pull request Oct 26, 2024

Add cli-arg service-max-value-rows #1589

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a VALUES clause to the query of a SERVICE clause to simplify the execution #1341

Add a VALUES clause to the query of a SERVICE clause to simplify the execution #1341

UNEXENU commented May 8, 2024 •

edited by joka921

Loading

joka921 left a comment

joka921 May 10, 2024

joka921 May 10, 2024

joka921 May 10, 2024

joka921 May 10, 2024

joka921 May 10, 2024

joka921 May 10, 2024

joka921 May 10, 2024

codecov bot commented May 10, 2024 •

edited

Loading

UNEXENU May 16, 2024 •

edited

Loading

joka921 May 16, 2024

joka921 May 16, 2024

UNEXENU May 16, 2024

joka921 May 16, 2024

joka921 left a comment

joka921 May 16, 2024

joka921 May 16, 2024

joka921 May 16, 2024

joka921 May 16, 2024

joka921 May 16, 2024

joka921 May 16, 2024

joka921 May 16, 2024

UNEXENU commented May 17, 2024

joka921 commented May 17, 2024

joka921 left a comment

joka921 May 21, 2024

UNEXENU May 28, 2024

joka921 May 21, 2024

joka921 left a comment

joka921 May 28, 2024

joka921 May 28, 2024

sonarqubecloud bot commented May 31, 2024

	if (aRootOp == bRootOp) {
	if (static_cast<bool>(aRootOp) == static_cast<bool>(bRootOp)) {

Add a VALUES clause to the query of a SERVICE clause to simplify the execution #1341

Add a VALUES clause to the query of a SERVICE clause to simplify the execution #1341

Conversation

UNEXENU commented May 8, 2024 • edited by joka921 Loading

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 10, 2024 • edited Loading

Codecov Report

UNEXENU May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UNEXENU commented May 17, 2024

joka921 commented May 17, 2024

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented May 31, 2024

Quality Gate passed

UNEXENU commented May 8, 2024 •

edited by joka921

Loading

codecov bot commented May 10, 2024 •

edited

Loading

UNEXENU May 16, 2024 •

edited

Loading