A lazy parser for the `application/sparql-results+json` format #1412

UNEXENU · 2024-07-23T08:22:42Z

This parser can be used to lazily parse JSON input where most of the data resides in a single JSON array, the path to which is known in advance with the additional assumption that the single entries of that array are small, but there might be many of them. This assumption holds for the JSON format of SPARQL query results, which consist of some (small) metadata like the contained variables and a large array that contains the result rows, each of which are typically small (at most one entry per variable).
In the future this parser will be used to implement a lazy SERVICE operation.

joka921

first comments.

joka921 · 2024-07-23T12:21:03Z

src/util/LazyJsonParser.cpp

+        if (inArrayPath_) {
+          continue;
+        }
+        inString_ = !inString_;
+
+        if (expectKey_) {
+          if (inString_) {
+            strStart = it + 1;
+          } else {
+            absl::StrAppend(&tempKey_, std::string(strStart, it));
+            setKeysValueType_ = true;
+            expectKey_ = false;
+          }
+        }


Suggested change

if (inArrayPath_) {

continue;

}

inString_ = !inString_;

if (expectKey_) {

if (inString_) {

strStart = it + 1;

} else {

absl::StrAppend(&tempKey_, std::string(strStart, it));

setKeysValueType_ = true;

expectKey_ = false;

}

}

std::optional<string_view> s = parseString(it, end);

// Do something with the key.

codecov · 2024-07-29T10:49:26Z

Codecov Report

Attention: Patch coverage is 98.77301% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.47%. Comparing base (be816a9) to head (9b760ea).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/util/LazyJsonParser.cpp	98.77%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1412      +/-   ##
==========================================
+ Coverage   89.41%   89.47%   +0.06%     
==========================================
  Files         346      347       +1     
  Lines       25350    25513     +163     
  Branches     3381     3433      +52     
==========================================
+ Hits        22667    22829     +162     
  Misses       1494     1494              
- Partials     1189     1190       +1

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

joka921

A first thorough round of reviews.

joka921 · 2024-08-14T06:15:08Z

src/util/LazyJsonParser.cpp

+
+LazyJsonParser::LazyJsonParser(std::vector<std::string> arrayPath)


Suggested change

LazyJsonParser::LazyJsonParser(std::vector<std::string> arrayPath)

// ____________________________________________________________________________________

LazyJsonParser::LazyJsonParser(std::vector<std::string> arrayPath)

(Everywhere in .cpp files)

joka921 · 2024-08-14T06:15:17Z

src/util/LazyJsonParser.cpp

@@ -0,0 +1,159 @@
+#include "LazyJsonParser.h"


global include paths.

joka921 · 2024-08-14T06:15:36Z

src/util/LazyJsonParser.cpp

+namespace ad_utility {
+
+LazyJsonParser::LazyJsonParser(std::vector<std::string> arrayPath)
+    : arrayPath_(arrayPath),


Suggested change

: arrayPath_(arrayPath),

: arrayPath_(std::move(arrayPath)),

joka921 · 2024-08-14T06:19:59Z

src/util/LazyJsonParser.cpp

+  auto tryAddKeyToPath = [this]() {
+    if (strEnd_ != 0) {
+      curPath_.emplace_back(input_.substr(strStart_, strEnd_ - strStart_));
+    }
+  };


This can be a documented private member function.

src/util/LazyJsonParser.cpp

joka921 · 2024-08-14T06:51:45Z

src/util/LazyJsonParser.h

+  // Parses a chunk of JSON data and returns it with reconstructed structure.
+  std::string parseChunk(const std::string& inStr);


Suggested change

// Parses a chunk of JSON data and returns it with reconstructed structure.

std::string parseChunk(const std::string& inStr);

// Parses a chunk of JSON data and returns it with reconstructed structure.

std::string parseChunk(std::string_view inStr);

joka921 · 2024-08-14T06:59:29Z

src/util/LazyJsonParser.h

+  // TODO<unexenu> this should take a generator as argument instead
+  static std::string parse(const std::string& s,


You can take an auto&& for the first part.
We can then constrain this template to be some kind of range that produces some kinds of strings,
but that is maybe for another time.

joka921 · 2024-08-14T07:00:11Z

src/util/LazyJsonParser.h

+  std::string input_;
+  bool isEscaped_{false};
+  bool inString_{false};
+  bool inArrayPath_{false};
+  int openBracesInArrayPath_{0};
+  int openBrackets_{0};
+  unsigned int yieldCount_{0};
+
+  size_t strStart_{0};
+  size_t strEnd_{0};
+
+  std::vector<std::string> curPath_;
+  const std::vector<std::string> arrayPath_;
+
+  const std::string prefixInArray_;
+  const std::string suffixInArray_;
+};


These need comments (but first clean up the structure as suggested.

joka921 · 2024-08-14T07:02:30Z

src/util/LazyJsonParser.h

+                           const std::vector<std::string>& arrayPath) {
+    LazyJsonParser p(arrayPath);
+    return p.parseChunk(s);
+  }


We also need some safety measure to make sure that we cannot haven an denial of service attack with ill-formed JSON
(basically some thresholds like "after 1 Megabyte we must be in the array path, each entry in the arrayPath must be at most 1 MB and the same goes for everything after the array path.

This should be relatively easy to track.

I've added a simple check for that in the latest commit.

While it's possible to compute this precisely, the current solution compares the first/last element in the arrayPath together with the part before/after the arrayPath against the threshold.
All other elements are checked one by one.

This leads to the first/last element of arrayPath being more restricted in size than the other elements.

This mechanism now looks good to me.

test/LazyJsonParserTest.cpp

joka921

This is much much better,
I can now read and understand this code:)

I have some suggestions, but we are getting there!

joka921 · 2024-08-21T07:47:21Z

src/util/LazyJsonParser.h

+  static cppcoro::generator<std::string> parse(
+      cppcoro::generator<std::string> partJson,
+      std::vector<std::string> arrayPath) {
+    LazyJsonParser p(arrayPath);
+    for (const auto& chunk : partJson) {
+      co_yield p.parseChunk(chunk);
+    }
+  }


Shouldn't this generator yield JSON objects, to make it more typesafe?
Just add a the json::parse call into this function (same for the parse overload below).

joka921 · 2024-08-21T07:48:10Z

src/util/LazyJsonParser.h

+  // As above just on a single string.
+  static std::string parse(const std::string& s,
+                           const std::vector<std::string>& arrayPath) {
+    LazyJsonParser p(arrayPath);
+    return p.parseChunk(s);
+  }


What is the point of this function? Do we still need it? Otherwise it is best removed.

joka921 · 2024-08-21T07:55:13Z

src/util/LazyJsonParser.cpp

+    switch (state_.index()) {
+      case 0:


Use a cascade of if (holds_alternative) here, nobody knows what index 1 means:)

joka921 · 2024-08-21T07:56:43Z

src/util/LazyJsonParser.cpp

+        if (arrayPath_.empty() &&
+            std::holds_alternative<AfterArrayPath>(state_)) {
+          materializeEnd = idx;
+        }


This looks like something that should be inside parseInArrayPath,
because here we don't have the necessary context to understand it.

joka921 · 2024-08-21T08:05:59Z

src/util/LazyJsonParser.cpp

+    if (idx >= 1'000'000) {
+      throw std::runtime_error(
+          "Ill-formed JSON: Header size exceeds 1 Megabyte.");
+    }


This mechanism only catches the case
if the single chunks are very big.
That however is not the big problem, because that is the user's responsibility.
The problem is rather, if we parse many chunks without seeing the end of the header, or the end of an array.

The solution to this is basically also simple:
After each chunk (can be done in the very outer loop) Assert that the size of the remaining input (that you have to carry on to the next chunk as a prefix) is smaller than the given threshold if that makes sense.
So no need to add something within the single chunks, if someone wants to pass in megabytes here, this is not our problem:)

joka921 · 2024-08-21T08:12:00Z

src/util/LazyJsonParser.cpp

+        break;
+      case ',':
+        if (state.openBracketsAndBraces_ == 0) {
+          materializeEnd = idx;


As this materializeEnd is not read, but only written, it should be a return value of this function
(together with a comment, what this function returns).

joka921 · 2024-08-21T08:13:05Z

src/util/LazyJsonParser.cpp

+    if (idx - state.arrayEndIdx_ > 1'000'000) {
+      throw std::runtime_error(
+          "Ill-formed JSON: Element size exceeds 1 Megabyte.");
+    }


joka921 · 2024-08-21T08:14:01Z

src/util/LazyJsonParser.cpp

+        state.remainingBraces_ -= 1;
+        if (state.remainingBraces_ == 0) {
+          // End reached.
+          materializeEnd = idx + 1;


Same here, should be a return value.

joka921 · 2024-08-21T08:20:55Z

src/util/LazyJsonParser.cpp

+  absl::StrAppend(&res, input_.substr(0, materializeEnd));
+  input_ = input_.substr(std::min(materializeEnd + 1, input_.size()));


This implicitly skips one character.
It is probably correct, but it looks really fishy.
This either needs documentation (why is this correct in all cases), or has to be fixed.
Is it correct , that this only happens either at a , inside the ArrayPath, or completely at the end after the trailer when the input is done.
So you can assert something like (inArrayPath && skippedCharIsComm) || ( afterArrayPath + materializeEnd is really at the end).

joka921 · 2024-08-21T08:24:04Z

src/util/LazyJsonParser.cpp

+  if (litLength_ > 0) {
+    curPath_.emplace_back(input.substr(litStart_, litLength_));
+    // The marked literal got consumed/added as key -> reset litLength_.
+    litLength_ = 0;


Actually: Are the "" part of the literal or not?
Otherwise your case litLength_ == 0 is ambigous (no literal vs. empty literal).
I think the much cleaner interface is to store the
litStart_ and litLenght_ in a struct and wrap this in an std::optional<> s.t. you really have a clear "there just was no literal" case.

joka921

Some very minor suggestions,
This is really looking good now.

joka921 · 2024-08-27T10:59:47Z

src/util/LazyJsonParser.h

      cppcoro::generator<std::string> partJson,
      std::vector<std::string> arrayPath) {
    LazyJsonParser p(arrayPath);
    for (const auto& chunk : partJson) {
-      co_yield p.parseChunk(chunk);
+      if (auto res = p.parseChunk(chunk); res.has_value()) {
+        co_yield res;


Previously, thsi box contained nonsense because I didn't understand the code.

This is not a template and not super performance critical,
You can move the definition to the .cpp file.

joka921 · 2024-08-27T11:03:16Z

src/util/LazyJsonParser.cpp

@@ -67,16 +63,13 @@ void LazyJsonParser::parseLiteral(size_t& idx) {
  if (input_[idx] == '"' && !inLiteral_) {
    ++idx;
    if (std::holds_alternative<BeforeArrayPath>(state_)) {
-      std::get<BeforeArrayPath>(state_).litStart_ = idx;
+      std::get<BeforeArrayPath>(state_).optLiteral_ =
+          BeforeArrayPath::LiteralView{.start_ = idx};


An explicit .length_ = 0 would also be nice, then one doesn't have to look that up and it doesn't hurt.

joka921 · 2024-08-27T11:04:40Z

src/util/LazyJsonParser.cpp

+          std::get<BeforeArrayPath>(state_).optLiteral_->length_ =
+              idx - std::get<BeforeArrayPath>(state_).optLiteral_->start_;


use optLiteral.value().lent_ , some for start .
These unsafe accesses give me goosebumps.

joka921 · 2024-08-27T11:08:13Z

src/util/LazyJsonParser.cpp

  if (yieldCount_ > 0) {
-    res = prefixInArray_;
+    resStr = prefixInArray_;


Can use a ternary instead of three lines: std::string resStr = yeldCount_ >0 ? prefixInArray_ : "";

joka921 · 2024-08-27T11:10:45Z

src/util/LazyJsonParser.h

+                           const std::vector<std::string>& arrayPath) {
+    LazyJsonParser p(arrayPath);
+    return p.parseChunk(s);
+  }


This mechanism now looks good to me.

joka921 · 2024-08-27T11:13:43Z

test/LazyJsonParserTest.cpp

+  EXPECT_NO_THROW(expectYields("[1,2,3]", {}, arrayPath));
+  EXPECT_NO_THROW(expectYields("{}", {}, arrayPath));
+
+  EXPECT_ANY_THROW(expectYields(std::string(1'000'000, '0'), {}, arrayPath));


The yellow branches inside the Ad_CORRECTNESS_CHECKs seem to indicate that you haven't tested the case, that
e.g. there is a literal that is between a chunk.
But then again this looks odd to me, because you yield single characters.
Maybe we can both have a look at this.

… of the main object.

joka921

Very nice!
Only two tiny suggestions (best practices)
and then this is ready to go!

joka921 · 2024-08-28T19:08:13Z

src/util/LazyJsonParser.cpp

+cppcoro::generator<nlohmann::json> LazyJsonParser::parse(
+    cppcoro::generator<std::string> partJson,
+    std::vector<std::string> arrayPath) {
+  LazyJsonParser p(arrayPath);


Suggested change

LazyJsonParser p(arrayPath);

LazyJsonParser p{std::move(arrayPath});

joka921 · 2024-08-28T19:09:26Z

src/util/LazyJsonParser.cpp

+  for (const auto& chunk : partJson) {
+    if (auto res = p.parseChunk(chunk); res.has_value()) {
+      co_yield res;
+      if (p.endReached_) {


As this is a (albeit static) member function of LazyJsonParser, endReached_ can (as all the other member variabls) be private, which then also gets rid of the Sonarcloud message.

joka921 · 2024-08-28T19:10:52Z

src/util/LazyJsonParser.h

+      std::vector<std::string> arrayPath);
+
+  // Indicates whether the end of the object has been reached.
+  bool endReached_{false};


As stated above, I am pretty sure that this can be private.

sonarqubecloud · 2024-08-29T09:09:07Z

Quality Gate passed

Issues
0 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

joka921

Very nice! This is a milestone towards more RAM (and possibly time-efficient) SERVICE handling.

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.

Base implementation of the LazyJsonParser.

b50aff3

joka921 reviewed Jul 23, 2024

View reviewed changes

separated different parsing sections, simplified path stack

bd0f993

UNEXENU added 3 commits July 30, 2024 15:57

fixed issues, improved test coverage

d51d600

more fixes

fc32b7b

added parse coroutine method

1ace9e7

joka921 requested changes Aug 14, 2024

View reviewed changes

UNEXENU added 5 commits August 14, 2024 21:13

Divided parsing into 3 sections to improve readability

b3b0bb3

added simple size limitation for input sections + other fixes

55b8276

improved key detection

38256d4

Merge branch 'ad-freiburg:master' into lazy-json-parser

b26a80e

improved accuracy of size limitation measurements + code coverage

6b727ba

joka921 requested changes Aug 21, 2024

View reviewed changes

fixed input-threshold, literal cornercase etc.

dd4f999

joka921 requested changes Aug 27, 2024

View reviewed changes

UNEXENU added 3 commits August 27, 2024 17:50

Fixed bug where the parser would keep reading the input after the end…

7641d4a

… of the main object.

Merge remote-tracking branch 'upstream/master' into lazy-json-parser

1eb0833

fix typo

3c2a91b

joka921 requested changes Aug 28, 2024

View reviewed changes

UNEXENU added 2 commits August 29, 2024 09:47

Merge remote-tracking branch 'upstream/master' into lazy-json-parser

c58c5bd

minor fixes

9b760ea

joka921 approved these changes Aug 29, 2024

View reviewed changes

joka921 changed the title ~~Implement LazyJsonParser~~ A lazy parser for the application/sparql-results+json format Aug 29, 2024

joka921 merged commit 7dcfea2 into ad-freiburg:master Aug 29, 2024
20 checks passed

UNEXENU mentioned this pull request Sep 9, 2024

Lazy import for SERVICE #1491

Merged


		LazyJsonParser::LazyJsonParser(std::vector<std::string> arrayPath)

		// Parses a chunk of JSON data and returns it with reconstructed structure.
		std::string parseChunk(const std::string& inStr);

		// TODO<unexenu> this should take a generator as argument instead
		static std::string parse(const std::string& s,

		absl::StrAppend(&res, input_.substr(0, materializeEnd));
		input_ = input_.substr(std::min(materializeEnd + 1, input_.size()));

		std::get<BeforeArrayPath>(state_).optLiteral_->length_ =
		idx - std::get<BeforeArrayPath>(state_).optLiteral_->start_;

	LazyJsonParser p(arrayPath);
	LazyJsonParser p{std::move(arrayPath});

A lazy parser for the application/sparql-results+json format #1412

A lazy parser for the application/sparql-results+json format #1412

Conversation

UNEXENU commented Jul 23, 2024 • edited by joka921 Loading

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2024 • edited Loading

Codecov Report

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

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

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

sonarqubecloud bot commented Aug 29, 2024

Quality Gate passed

joka921 left a comment

Choose a reason for hiding this comment

A lazy parser for the `application/sparql-results+json` format #1412

A lazy parser for the `application/sparql-results+json` format #1412

UNEXENU commented Jul 23, 2024 •

edited by joka921

Loading

codecov bot commented Jul 29, 2024 •

edited

Loading