Lazy import for SERVICE #1491

UNEXENU · 2024-09-09T17:12:43Z

Integrate the LazyJsonParser introduced in #1412 into the SERVICE Operation, which will help to reduce RAM usage for the import of large results. In particular, the (possibly large) JSON result of a SERVICE will not be fully materialized, but converted to a (possibly much smaller) IdTable on the fly. This is a preparation for making the SERVICE operation completely lazy.

…rameters

codecov · 2024-09-09T17:47:19Z

Codecov Report

Attention: Patch coverage is 99.22481% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.15%. Comparing base (dc71166) to head (9fa1043).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
src/util/LazyJsonParser.cpp	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1491      +/-   ##
==========================================
- Coverage   94.15%   94.15%   -0.01%     
==========================================
  Files         347      348       +1     
  Lines       25627    25698      +71     
  Branches     3445     3453       +8     
==========================================
+ Hits        24130    24196      +66     
- Misses       1455     1460       +5     
  Partials       42       42

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

joka921

In general this already looks very nice.
There are some small improvements that I have suggested.

src/engine/Service.cpp

src/util/LazyJsonParser.cpp

joka921 · 2024-09-11T07:34:19Z

test/ServiceTest.cpp

@@ -202,6 +202,7 @@ TEST_F(ServiceTest, computeResult) {
      [&](const std::string& result, std::string_view errorMsg,
          boost::beast::http::status status = boost::beast::http::status::ok,
          std::string contentType = "application/sparql-results+json") {
+        LOG(INFO) << "MSG: " << errorMsg << '\n';


That seems like some debugging stuff that can be removed?

joka921

I really like the direction where this is headed, only a few small things are missing.

joka921 · 2024-09-12T15:19:29Z

src/util/LazyJsonParser.h

@@ -20,15 +20,19 @@ namespace ad_utility {
 */
 class LazyJsonParser {
 public:
+  // Generator detail, the first 100 input characters for better error context.
+  struct Details {
+    std::string first100_;


I think we also want to have the last 100 characters in addition,
it might also happen, that everything starts alright, but in the middle something goes wrong, we also need context for that.

joka921 · 2024-09-12T15:20:43Z

src/util/LazyJsonParser.cpp

        for (const auto& bytes : partialJson) {
-          co_yield reinterpret_cast<const char*>(bytes.data());
+          co_yield std::string(reinterpret_cast<const char*>(bytes.data()),


Suggested change

co_yield std::string(reinterpret_cast<const char*>(bytes.data()),

co_yield std::string_view(reinterpret_cast<const char*>(bytes.data()),

joka921 · 2024-09-12T15:23:07Z

src/engine/Service.cpp

+// ____________________________________________________________________________
+void Service::verifyVariables(
+    const nlohmann::json& j,
+    const ad_utility::LazyJsonParser::Generator& gen) const {


It suffices to pass const Details& as the second argument, I was confused what you need the generator for here and how this should work:)

joka921 · 2024-09-12T15:25:42Z

src/engine/Service.cpp

+    throwErrorWithContext("JSON result does not have the expected structure",
+                          gen.details().first100_);


You could also include the complete "head" clause in this error message, because that's what causes the problem here, so something like
... does not have the expected structere, as its "head" section is not according to the SPARQL standard. The "head"section is" .... dump the subjson here.

joka921 · 2024-09-12T15:27:18Z

src/engine/Service.cpp

+
+  if (responseVars != expectedVars) {
+    throwErrorWithContext(
+        absl::StrCat("Header row of JSON result for SERVICE query is \"",


In this message (as well as the one before) also add a "Probable cause: the remote endpoint sent a JSON response that is not according to the SPARQL standard".

joka921 · 2024-09-12T15:29:49Z

src/engine/Service.cpp

-        ">: ", sv, ". First 100 bytes of the response: ",
-        ctx.substr(0, std::min(100, (int)ctx.size()))));
+    this->throwErrorWithContext(sv,
+                                ctx.substr(0, std::min(100, (int)ctx.size())));


Suggested change

ctx.substr(0, std::min(100, (int)ctx.size())));

ctx.substr(0, 100));

substr automatically handles the out-of-bounds case.

joka921 · 2024-09-12T15:30:21Z

src/engine/Service.cpp

  };

  // Verify status and content-type of the response.
  if (response.status_ != boost::beast::http::status::ok) {
+    LOG(INFO) << serviceUrl.asString() << '\n';


Should that remaing here, or was that just for debugging?

joka921 · 2024-09-12T15:32:41Z

src/engine/Service.cpp

+  if (!resultExists || !varsChecked) {
+    throwErrorWithContext("JSON result does not have the expected structure",
+                          body.details().first100_);


You can make two distinct checks with distinct error messages
(head section was missing) or results section was missing.

joka921 · 2024-09-12T15:37:57Z

test/ServiceTest.cpp

+  expectThrowOrSilence(
+      "{\"head\": {\"vars\": 1},"
+      "\"results\": {\"bindings\": {}}}",
+      "JSON result does not have the expected structure.");


I think a test case for results section missing and results section has the wrong structure is missing.
In particular we probably want to catch the internal exceptions of the LazyJson parser and make something user-friendly out of them. (People will complain to us, even if its the fault of the other endpoint, so we should help our future selves as much as possible.

joka921

Some small things left.

joka921 · 2024-09-13T13:42:29Z

src/util/LazyJsonParser.cpp

+        details.last100_ = chunk.substr(
+            std::max(0, static_cast<int>(chunk.size() - 100)), 100);


I think we want to have the last 100 always.
So overwrite them directly after setting the first100, but outside the if of course.
That way we also get the context if the parsing of the chunk failed, which is probably the most useful.

joka921 · 2024-09-13T13:52:30Z

src/util/LazyJsonParser.cpp

@@ -250,7 +252,7 @@ std::optional<nlohmann::json> LazyJsonParser::constructResultFromParsedChunk(
  size_t nextChunkStart =
      materializeEnd == 0 ? 0 : std::min(materializeEnd + 1, input_.size());
  if (input_.size() - nextChunkStart >= 1'000'000) {
-    throw std::runtime_error("Ill formed Json.");
+    throw Error("Ill formed JSON.");


This message should be something like
QLever currently doesn't support SERVICE results where a single result row is larger than 1 MB.

sonarqubecloud · 2024-09-13T15:53:01Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

UNEXENU added 2 commits September 9, 2024 17:37

Fixed bug that didn't allow SERVICE endpoints to send Content-Type pa…

e4f930d

…rameters

Use the LazyJsonParser in SERVICE Operation

89cbb8e

UNEXENU closed this Sep 9, 2024

UNEXENU reopened this Sep 9, 2024

joka921 requested changes Sep 11, 2024

View reviewed changes

UNEXENU added 3 commits September 11, 2024 17:40

various improvements

b7f1f9b

added previously existing context for error messages

b63e130

Merge remote-tracking branch 'upstream/master' into service-lazy-import

8af90ec

joka921 requested changes Sep 12, 2024

View reviewed changes

added context to parser errors

4427fff

joka921 requested changes Sep 13, 2024

View reviewed changes

UNEXENU added 2 commits September 13, 2024 16:14

Merge remote-tracking branch 'upstream/master' into service-lazy-import

e92e514

small fixes

9fa1043

joka921 approved these changes Sep 18, 2024

View reviewed changes

joka921 merged commit b70df93 into ad-freiburg:master Sep 18, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy import for SERVICE #1491

Lazy import for SERVICE #1491

UNEXENU commented Sep 9, 2024 •

edited by joka921

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

joka921 left a comment

joka921 Sep 11, 2024

joka921 left a comment

joka921 Sep 12, 2024

joka921 Sep 12, 2024

joka921 Sep 12, 2024

joka921 Sep 12, 2024

joka921 Sep 12, 2024

joka921 Sep 12, 2024

joka921 Sep 12, 2024

joka921 Sep 12, 2024

joka921 Sep 12, 2024

joka921 left a comment

joka921 Sep 13, 2024

joka921 Sep 13, 2024

sonarqubecloud bot commented Sep 13, 2024

	co_yield std::string(reinterpret_cast<const char*>(bytes.data()),
	co_yield std::string_view(reinterpret_cast<const char*>(bytes.data()),

		throwErrorWithContext("JSON result does not have the expected structure",
		gen.details().first100_);

	ctx.substr(0, std::min(100, (int)ctx.size())));
	ctx.substr(0, 100));

		details.last100_ = chunk.substr(
		std::max(0, static_cast<int>(chunk.size() - 100)), 100);

Lazy import for SERVICE #1491

Lazy import for SERVICE #1491

Conversation

UNEXENU commented Sep 9, 2024 • edited by joka921 Loading

codecov bot commented Sep 9, 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

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 Sep 13, 2024

Quality Gate passed

UNEXENU commented Sep 9, 2024 •

edited by joka921

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading