Only compute the sibling of a `SERVICE` once #1556

UNEXENU · 2024-10-15T13:04:45Z

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.

codecov · 2024-10-15T14:34:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.07%. Comparing base (db089f8) to head (d991ef1).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1556      +/-   ##
==========================================
+ Coverage   89.03%   89.07%   +0.03%     
==========================================
  Files         368      368              
  Lines       34183    34227      +44     
  Branches     3860     3866       +6     
==========================================
+ Hits        30434    30486      +52     
+ Misses       2482     2480       -2     
+ Partials     1267     1261       -6

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

joka921

The initial approach looks clean (especially not too much code), but we need support for laziness.
Let me know if you need help etc.

joka921 · 2024-10-16T13:36:08Z

src/engine/Service.cpp

+// ____________________________________________________________________________
+void Service::precomputeSiblingResult(std::shared_ptr<Operation> left,
+                                      std::shared_ptr<Operation> right,
+                                      bool rightOnly) {
+  AD_CORRECTNESS_CHECK(left && right);
+  auto a = std::dynamic_pointer_cast<Service>(left);
+  auto b = std::dynamic_pointer_cast<Service>(right);
+
+  if ((rightOnly && !b) ||
+      (!rightOnly && static_cast<bool>(a) == static_cast<bool>(b))) {
+    return;
+  }
+
+  const auto& [service, sibling] = [&]() {
+    if (a) {
+      return std::tie(a, right);
+    } else {
+      AD_CORRECTNESS_CHECK(b);
+      return std::tie(b, left);
+    }
+  }();
+
+  auto siblingResult = sibling->getResult();
+  sibling->precomputedResultBecauseSiblingOfService_ = siblingResult;
+  if (siblingResult->idTable().size() <=
+      RuntimeParameters().get<"service-max-value-rows">()) {
+    service->precomputedSiblingResult_ = siblingResult;
+  }
+}


Although currently neither of the JOIN-like operations support laziness, we still need to prepare the support here.
This function should take another parameter requestLaziness which is then passed to the getResult() call.
If the result is in fact lazy (akak not fullyMaterialized, then you basically store the returned IdTables until either you've consumed everything, or you have too many rows. Then you create a new result with a new generator that first yields all the blocks you've already retrieved and then continues consuming the downstream generator.
For the service result you can just concatenate the tables to a single IdTable to get a materialized result.

…-computation

…o in Service

…-computation

joka921

A pass on everything but the tests.

joka921 · 2024-10-24T15:03:28Z

src/engine/OptionalJoin.cpp

@@ -92,6 +93,12 @@ string OptionalJoin::getDescriptor() const {
 ProtoResult OptionalJoin::computeResult([[maybe_unused]] bool requestLaziness) {
  LOG(DEBUG) << "OptionalJoin result computation..." << endl;

+  // If the right of the RootOperations is a Service, precompute it's sibling's


Suggested change

// If the right of the RootOperations is a Service, precompute it's sibling's

// If the right of the RootOperations is a Service, precompute the result of its sibling.

Similar typo (its vs it's ) in the other operations.

joka921 · 2024-10-24T15:06:19Z

src/engine/Service.cpp

+  auto skipSortOperation = [](std::shared_ptr<Operation>& op) {
+    auto children = op->getChildren();
+    if (children.size() == 1) {
+      op = children[0]->getRootOperation();
+    }
+  };


That is definitely wrong. There are many other operations with only one child, in particular GROUP BY, DISTINCT, FILTER.... So you have to check whether it's a sort via a dynamic cast.

joka921 · 2024-10-24T15:08:28Z

src/engine/Service.cpp

+  if ((rightOnly && !static_cast<bool>(b)) ||
+      (!rightOnly && static_cast<bool>(a) == static_cast<bool>(b))) {
+    return;
+  }


If both are service requests, you could still in principle compute one of them here, and hope that it's the smaller one.
At least some comment should be left here. IF in the future we figure out a way to better estimate the result size of a service, we could then use the smaller estimate here. But that's not important for now.
As said: A comment why we currently don't support two services should be there at least.

joka921 · 2024-10-24T15:10:48Z

src/engine/Service.cpp

+                             : ComputationMode::FULLY_MATERIALIZED);
+
+  if (siblingResult->isFullyMaterialized()) {
+    sibling->precomputedResultBecauseSiblingOfService_ = siblingResult;


You could do this step as the second one, then you can std::move(...) the siblingResult.

joka921 · 2024-10-24T15:11:58Z

src/engine/Service.cpp

+         cppcoro::generator<Result::IdTableVocabPair> prevGenerator,
+         cppcoro::generator<Result::IdTableVocabPair>::iterator it)
+      -> cppcoro::generator<Result::IdTableVocabPair> {


I think there is an alias inside the Result class for this long type (Result::Generator) or something, please use it.

joka921 · 2024-10-24T15:13:21Z

src/engine/Service.cpp

+    rows += pair.idTable_.size();
+    resultPairs.push_back(std::move(pair));
+
+    if (rows > RuntimeParameters().get<"service-max-value-rows">()) {


I think you could access that constant once in advance, and not in the loop. (minor optimization).

joka921 · 2024-10-24T15:14:49Z

src/engine/Service.cpp

+              sibling->getExecutionContext()->getAllocator()},
+      LocalVocab{});
+
+  for (auto& pair : resultPairs) {


You know or can easily compute the total number of rows and the reserve the idTable.

joka921 · 2024-10-24T15:16:19Z

src/engine/Service.h

@@ -109,11 +95,20 @@ class Service : public Operation {
  static std::optional<std::string> idToValueForValuesClause(
      const Index& index, Id id, const LocalVocab& localVocab);

+  // Given two child-operations of a Join-, OptionalJoin- or Minus operation,
+  // this method tries to precompute the result of one if the other one
+  // (it's sibling) is a Service operation.


Suggested change

// (it's sibling) is a Service operation.

// (its sibling) is a Service operation.

joka921 · 2024-10-24T15:16:54Z

src/engine/Service.h

+  // Given two child-operations of a Join-, OptionalJoin- or Minus operation,
+  // this method tries to precompute the result of one if the other one
+  // (it's sibling) is a Service operation.
+  // If rightOnly is true (used by OptionalJoin and Minus),


Suggested change

// If rightOnly is true (used by OptionalJoin and Minus),

// If `rightOnly` is true (used by `OptionalJoin` and `Minus`),

(try to use this convention also consistently for your other comments)

joka921 · 2024-10-24T18:42:14Z

test/OperationTest.cpp

+      idTable.clone(), std::vector<ColumnIndex>{0}, LocalVocab{});
+  operation.precomputedResultBecauseSiblingOfService_ =
+      std::make_optional(result);
+  ASSERT_EQ(operation.getResult(), result);


You can also test that it is cleared after returning it.

…-computation

joka921

Two very very minor nitpicky requests.
Almost ready to merge, great!

src/engine/Operation.h

joka921 · 2024-10-26T07:21:34Z

src/engine/Service.cpp

-    auto children = op->getChildren();
-    if (children.size() == 1) {
+    if (static_cast<bool>(std::dynamic_pointer_cast<Sort>(op))) {
+      auto children = op->getChildren();


Suggested change

auto children = op->getChildren();

const auto& children = op->getChildren();

probably suffices (assuming that getRootOperation is const. else leave it and tell me).

…-computation

sparql-conformance · 2024-10-26T08:49:44Z

Conformance check passed ✅

No test result changes.

Details: https://qlever.cs.uni-freiburg.de/sparql-conformance-ui?cur=d991ef1a0c1ebba91f73e98f2a2e6500da073a81&prev=db089f88b6dc2d8b6ad782cb6edbadf718c60b6d

sonarqubecloud · 2024-10-26T10:14:59Z

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

joka921

Thanks!

This fixes #1659, an issue introduced in #1556 that made the precomputation of a `SERVICE` sibling in `JOIN` operations impossible.

added precomputation of siblingResult

d624cb6

joka921 requested changes Oct 16, 2024

View reviewed changes

UNEXENU added 7 commits October 21, 2024 17:41

Merge remote-tracking branch 'upstream/master' into service-avoid-2nd…

aa6c0d2

…-computation

removed siblingTree from QueryPlanner and replaced it with siblingInf…

a20bbde

…o in Service

fix

5a2764f

added Service runtimeinfo; fixed missing include

d9be4a1

Merge remote-tracking branch 'upstream/master' into service-avoid-2nd…

b251c9b

…-computation

Detect/precompute service hidden under Sort operation

3c07631

fix test

41bbebf

joka921 requested changes Oct 24, 2024

View reviewed changes

UNEXENU added 4 commits October 25, 2024 08:54

Merge remote-tracking branch 'upstream/master' into service-avoid-2nd…

89f5c72

…-computation

various improvements

8588e81

fix

95f4199

try again with explicit nullptr check

02f1033

joka921 requested changes Oct 26, 2024

View reviewed changes

UNEXENU added 2 commits October 26, 2024 10:17

Merge remote-tracking branch 'upstream/master' into service-avoid-2nd…

f1cc384

…-computation

minor changes

d991ef1

joka921 approved these changes Oct 26, 2024

View reviewed changes

joka921 changed the title ~~Fix computing SERVICE Sibling twice~~ Only compute the sibling of a SERVICE once Oct 26, 2024

joka921 merged commit 9e65d28 into ad-freiburg:master Oct 26, 2024
22 checks passed

hannahbast mentioned this pull request Dec 5, 2024

SERVICE with constrained outside no longer uses that constraint #1659

Closed

UNEXENU mentioned this pull request Dec 5, 2024

Fix precomputation for SERVICE in JOIN operation #1661

Merged

joka921 pushed a commit that referenced this pull request Dec 6, 2024

fix join-order s.t. precomputation of service works again. (#1661)

c76f24b

This fixes #1659, an issue introduced in #1556 that made the precomputation of a `SERVICE` sibling in `JOIN` operations impossible.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only compute the sibling of a `SERVICE` once #1556

Only compute the sibling of a `SERVICE` once #1556

UNEXENU commented Oct 15, 2024 •

edited by joka921

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

joka921 left a comment

joka921 Oct 16, 2024

joka921 left a comment

joka921 Oct 24, 2024

joka921 Oct 24, 2024

joka921 Oct 24, 2024

joka921 Oct 24, 2024

joka921 Oct 24, 2024

joka921 Oct 24, 2024

joka921 Oct 24, 2024

joka921 Oct 24, 2024

joka921 Oct 24, 2024

joka921 Oct 24, 2024

joka921 left a comment

joka921 Oct 26, 2024

sparql-conformance bot commented Oct 26, 2024

sonarqubecloud bot commented Oct 26, 2024 •

edited

Loading

joka921 left a comment

	// If the right of the RootOperations is a Service, precompute it's sibling's
	// If the right of the RootOperations is a Service, precompute the result of its sibling.

	// (it's sibling) is a Service operation.
	// (its sibling) is a Service operation.

	// If rightOnly is true (used by OptionalJoin and Minus),
	// If `rightOnly` is true (used by `OptionalJoin` and `Minus`),

	auto children = op->getChildren();
	const auto& children = op->getChildren();

Only compute the sibling of a SERVICE once #1556

Only compute the sibling of a SERVICE once #1556

Conversation

UNEXENU commented Oct 15, 2024 • edited by joka921 Loading

codecov bot commented Oct 15, 2024 • edited Loading

Codecov Report

joka921 left a 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

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

sparql-conformance bot commented Oct 26, 2024

Conformance check passed ✅

sonarqubecloud bot commented Oct 26, 2024 • edited Loading

Quality Gate passed

joka921 left a comment

Choose a reason for hiding this comment

Only compute the sibling of a `SERVICE` once #1556

Only compute the sibling of a `SERVICE` once #1556

UNEXENU commented Oct 15, 2024 •

edited by joka921

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

sonarqubecloud bot commented Oct 26, 2024 •

edited

Loading