feat(functions): Support for canonicalization of JSON #11284

kgpai · 2024-10-17T00:02:16Z

This is preliminary PR that adds support for canonicalization of JSON strings. This initial PR only tackles canonicalization of json_parse. Another diff will handle CAST( _ as JSON) . Canonicalization is required since currently Velox just treats JSON as varchars thus equivalent JSON but having different backing varchar's are treated as separate JSON's which is incorrect and contrary to behavior shown by Presto.

netlify · 2024-10-17T00:02:34Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`95d8911`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/673e5508704150000821107e

kgpai · 2024-10-17T00:03:02Z

The first version of the diff isnt optimized and creates an additional copy before writing to the buffer; It also uses stringstream etc for ease of use. I will be optimizing after ascertaining correctness.

gggrace14

Left some comments. Will leave the perf improvement suggestion to Jimmy and watch.

Thank you for making this change @kgpai Krishna! I think it will work functionally.

gggrace14 · 2024-10-17T22:21:32Z

velox/functions/prestosql/JsonFunctions.cpp

+          rows.end(),
+          stringViews,
+          std::vector<BufferPtr>{});
+      auto flatResult = localResult->asFlatVector<StringView>();


Is this line redundant with line 178, localResult=? Both return a pointer to FlatVector

gggrace14 · 2024-10-17T22:50:21Z

velox/functions/prestosql/JsonFunctions.cpp

+  JsonLeafView(const StringView view) : view_(view){};
+
+  void canonicalize(std::stringstream& stream) override {
+    stream << view_;


Here is where we are going to call the unicode escape func in the future, right?

That is right I will add your unicode escape when copying the string views out.

gggrace14 · 2024-10-17T23:11:03Z

velox/functions/prestosql/JsonFunctions.cpp

        }
+
+        jsonView = std::make_shared<JsonArrayView>(arrayPtr);


When looking at the constructor of JsonArrayView, we are copying the std::vector twice. What is the conventional way to do in this case?

In almost all cases the compiler, I think the compiler will end up doing copy elision, so this should not create a copy as these types are not marked volatile . However to further satisfy you , i can do an explicit std::move.

gggrace14 · 2024-10-17T23:17:54Z

velox/functions/prestosql/JsonFunctions.cpp

    if (!doc.at_end()) {
      return simdjson::TRAILING_CONTENT;
    }
    return simdjson::SUCCESS;
  }

  template <typename T>
-  static simdjson::error_code validate(T value) {
+  static simdjson::error_code validate(T value, JsonViewPtr& jsonView) {


It looks to me we're spreading the canonicalization changes to JsonView::canonicalize() and JsonParseFunction::validate(), with JsonParseFunction::validate() doing the whitespace trimming. Personally I would prefer to move all the canonicalization changes to JsonView::canonicalize() to be more modularized, and leave JsonParseFunction::validate() to do the simple validation as before.

Yes, Agreed, moving the trim etc to the canonicalize().

facebook-github-bot · 2024-10-28T20:24:35Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

velox/functions/prestosql/JsonFunctions.cpp

kgpai · 2024-10-28T23:07:07Z

@Yuhta Do you have concerns with using stringstream, and and instead directly memcpy'ing to some string buffer?

Yuhta · 2024-10-29T15:48:03Z

@kgpai Ideally we should calculate the exact output size beforehand and allocate it once then writing into it.

facebook-github-bot · 2024-10-30T22:53:43Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-11-01T21:55:21Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-11-01T22:25:45Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-11-01T22:36:36Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-11-04T19:39:57Z

This pull request was exported from Phabricator. Differential Revision: D65084925

Summary: This is preliminary PR that adds support for canonicalization of JSON strings. This initial PR only tackles canonicalization of json_parse. Another diff will handle CAST( _ as JSON) . Canonicalization is required since currently Velox just treats JSON as varchars thus equivalent JSON but having different backing varchar's are treated as separate JSON's which is incorrect and contrary to behavior shown by Presto. Differential Revision: D65084925 Pulled By: kgpai

velox/functions/prestosql/JsonFunctions.cpp

Yuhta · 2024-11-05T18:42:51Z

velox/functions/prestosql/JsonFunctions.cpp

@@ -84,38 +162,71 @@ class JsonParseFunction : public exec::VectorFunction {
      auto value = arg->as<ConstantVector<StringView>>()->valueAt(0);
      paddedInput_.resize(value.size() + simdjson::SIMDJSON_PADDING);
      memcpy(paddedInput_.data(), value.data(), value.size());
-      if (auto error = parse(value.size())) {
+      auto escapeSize = escapedStringSize(value.data(), value.size(), true);


Wonder is it safe to pass the JSON directly into escapedStringSize like this, since it is meant for string node value in JSON, not the JSON itself. It might work though since outside string nodes everything should be ASCII.

The escapeSize here is worst case size , Its hard to estimate the size only for the string nodes and then add the ascii parts ; I see no reason why this would be less than the size required, so I think this should be fine.

Yeah we can keep this for now and if weird bug showing up this is a suspicious point. Maybe leave a comment about the situation.

velox/functions/prestosql/JsonFunctions.cpp

Summary: This is preliminary PR that adds support for canonicalization of JSON strings. This initial PR only tackles canonicalization of json_parse. Another diff will handle CAST( _ as JSON) . Canonicalization is required since currently Velox just treats JSON as varchars thus equivalent JSON but having different backing varchar's are treated as separate JSON's which is incorrect and contrary to behavior shown by Presto. Reviewed By: Yuhta, gggrace14 Differential Revision: D65084925 Pulled By: kgpai

facebook-github-bot · 2024-11-17T20:32:25Z

This pull request was exported from Phabricator. Differential Revision: D65084925

Summary: This is preliminary PR that adds support for canonicalization of JSON strings. This initial PR only tackles canonicalization of json_parse. Another diff will handle CAST( _ as JSON) . Canonicalization is required since currently Velox just treats JSON as varchars thus equivalent JSON but having different backing varchar's are treated as separate JSON's which is incorrect and contrary to behavior shown by Presto. Reviewed By: Yuhta, gggrace14 Differential Revision: D65084925 Pulled By: kgpai

facebook-github-bot · 2024-11-17T20:33:34Z

This pull request was exported from Phabricator. Differential Revision: D65084925

…ator#11284) Summary: This is preliminary PR that adds support for canonicalization of JSON strings. This initial PR only tackles canonicalization of json_parse. Another diff will handle CAST( _ as JSON) . Canonicalization is required since currently Velox just treats JSON as varchars thus equivalent JSON but having different backing varchar's are treated as separate JSON's which is incorrect and contrary to behavior shown by Presto. Reviewed By: Yuhta, gggrace14 Differential Revision: D65084925 Pulled By: kgpai

facebook-github-bot · 2024-11-18T02:49:26Z

This pull request was exported from Phabricator. Differential Revision: D65084925

…ator#11284) Summary: This is preliminary PR that adds support for canonicalization of JSON strings. This initial PR only tackles canonicalization of json_parse. Another diff will handle CAST( _ as JSON) . Canonicalization is required since currently Velox just treats JSON as varchars thus equivalent JSON but having different backing varchar's are treated as separate JSON's which is incorrect and contrary to behavior shown by Presto. Reviewed By: Yuhta, gggrace14 Differential Revision: D65084925 Pulled By: kgpai

facebook-github-bot · 2024-11-18T21:48:20Z

This pull request was exported from Phabricator. Differential Revision: D65084925

…tor#11284) Summary: This is preliminary PR that adds support for canonicalization of JSON strings. This initial PR only tackles canonicalization of json_parse. Another diff will handle CAST( _ as JSON) . Canonicalization is required since currently Velox just treats JSON as varchars thus equivalent JSON but having different backing varchar's are treated as separate JSON's which is incorrect and contrary to behavior shown by Presto. Reviewed By: Yuhta, gggrace14 Differential Revision: D65084925 Pulled By: kgpai

facebook-github-bot · 2024-11-20T07:26:53Z

This pull request was exported from Phabricator. Differential Revision: D65084925

…tor#11284) Summary: This is preliminary PR that adds support for canonicalization of JSON strings. This initial PR only tackles canonicalization of json_parse. Another diff will handle CAST( _ as JSON) . Canonicalization is required since currently Velox just treats JSON as varchars thus equivalent JSON but having different backing varchar's are treated as separate JSON's which is incorrect and contrary to behavior shown by Presto. Reviewed By: Yuhta, gggrace14 Differential Revision: D65084925 Pulled By: kgpai

facebook-github-bot · 2024-11-20T17:58:50Z

This pull request was exported from Phabricator. Differential Revision: D65084925

…tor#11284) Summary: This is preliminary PR that adds support for canonicalization of JSON strings. This initial PR only tackles canonicalization of json_parse. Another diff will handle CAST( _ as JSON) . Canonicalization is required since currently Velox just treats JSON as varchars thus equivalent JSON but having different backing varchar's are treated as separate JSON's which is incorrect and contrary to behavior shown by Presto. Reviewed By: Yuhta, gggrace14 Differential Revision: D65084925 Pulled By: kgpai

facebook-github-bot · 2024-11-20T21:31:20Z

This pull request was exported from Phabricator. Differential Revision: D65084925

facebook-github-bot · 2024-11-20T23:17:25Z

@kgpai merged this pull request in de6a83d.

conbench-facebook · 2024-11-21T00:09:32Z

Conbench analyzed the 1 benchmark run on commit de6a83dc.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 17, 2024

kgpai requested review from Yuhta and gggrace14 October 17, 2024 00:03

kgpai force-pushed the json_parse_changes branch from 683f681 to 30a901e Compare October 17, 2024 00:07

gggrace14 reviewed Oct 17, 2024

View reviewed changes

kgpai force-pushed the json_parse_changes branch from 30a901e to 9bc80fb Compare October 28, 2024 19:43

Yuhta reviewed Oct 28, 2024

View reviewed changes

velox/functions/prestosql/JsonFunctions.cpp Outdated Show resolved Hide resolved

kgpai force-pushed the json_parse_changes branch 4 times, most recently from 49c10ef to 6859f21 Compare October 30, 2024 22:52

kgpai force-pushed the json_parse_changes branch from 6859f21 to 288d79f Compare November 1, 2024 21:46

kgpai changed the title ~~[Draft] [WIP] Add support for canonicalization of JSON.~~ Add support for canonicalization of JSON. Nov 1, 2024

kgpai force-pushed the json_parse_changes branch from 288d79f to d57b02c Compare November 1, 2024 22:24

kgpai force-pushed the json_parse_changes branch from d57b02c to 80fae98 Compare November 1, 2024 22:36

kgpai force-pushed the json_parse_changes branch from 80fae98 to 8b17860 Compare November 4, 2024 19:39

facebook-github-bot added the fb-exported label Nov 4, 2024

Yuhta reviewed Nov 5, 2024

View reviewed changes

velox/functions/prestosql/JsonFunctions.cpp Outdated Show resolved Hide resolved

kgpai force-pushed the json_parse_changes branch from 6d0d2b5 to fd1ddd0 Compare November 17, 2024 20:31

kgpai force-pushed the json_parse_changes branch from fd1ddd0 to 85f031c Compare November 17, 2024 20:33

kgpai force-pushed the json_parse_changes branch from 85f031c to f091c48 Compare November 18, 2024 02:48

kgpai changed the title ~~Add support for canonicalization of JSON.~~ feat(functions): Support for canonicalization of JSON. Nov 18, 2024

kgpai force-pushed the json_parse_changes branch from f091c48 to a9c2e21 Compare November 18, 2024 21:47

assignUser changed the title ~~feat(functions): Support for canonicalization of JSON.~~ feat(functions): Support for canonicalization of JSON Nov 18, 2024

kgpai changed the title ~~feat(functions): Support for canonicalization of JSON~~ feat(functions): Support for canonicalization of JSON. Nov 18, 2024

kgpai changed the title ~~feat(functions): Support for canonicalization of JSON.~~ feat(functions): Support for canonicalization of JSON Nov 18, 2024

kgpai force-pushed the json_parse_changes branch from a9c2e21 to 2ce62a9 Compare November 20, 2024 07:26

kgpai force-pushed the json_parse_changes branch from 2ce62a9 to ce082db Compare November 20, 2024 17:58

kgpai force-pushed the json_parse_changes branch from ce082db to 95d8911 Compare November 20, 2024 21:30

facebook-github-bot closed this in de6a83d Nov 20, 2024

facebook-github-bot added the Merged label Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(functions): Support for canonicalization of JSON #11284

feat(functions): Support for canonicalization of JSON #11284

kgpai commented Oct 17, 2024

netlify bot commented Oct 17, 2024 •

edited

Loading

kgpai commented Oct 17, 2024

gggrace14 left a comment •

edited

Loading

gggrace14 Oct 17, 2024

gggrace14 Oct 17, 2024

kgpai Oct 18, 2024

gggrace14 Oct 17, 2024

kgpai Oct 18, 2024

gggrace14 Oct 17, 2024

kgpai Oct 18, 2024

facebook-github-bot commented Oct 28, 2024

kgpai commented Oct 28, 2024

Yuhta commented Oct 29, 2024

facebook-github-bot commented Oct 30, 2024

facebook-github-bot commented Nov 1, 2024

facebook-github-bot commented Nov 1, 2024

facebook-github-bot commented Nov 1, 2024

facebook-github-bot commented Nov 4, 2024

Yuhta Nov 5, 2024

kgpai Nov 5, 2024

Yuhta Nov 7, 2024 •

edited

Loading

facebook-github-bot commented Nov 17, 2024

facebook-github-bot commented Nov 17, 2024

facebook-github-bot commented Nov 18, 2024

facebook-github-bot commented Nov 18, 2024

facebook-github-bot commented Nov 20, 2024

facebook-github-bot commented Nov 20, 2024

facebook-github-bot commented Nov 20, 2024

facebook-github-bot commented Nov 20, 2024

conbench-facebook bot commented Nov 21, 2024

feat(functions): Support for canonicalization of JSON #11284

feat(functions): Support for canonicalization of JSON #11284

Conversation

kgpai commented Oct 17, 2024

netlify bot commented Oct 17, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

kgpai commented Oct 17, 2024

gggrace14 left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 28, 2024

kgpai commented Oct 28, 2024

Yuhta commented Oct 29, 2024

facebook-github-bot commented Oct 30, 2024

facebook-github-bot commented Nov 1, 2024

facebook-github-bot commented Nov 1, 2024

facebook-github-bot commented Nov 1, 2024

facebook-github-bot commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuhta Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Nov 17, 2024

facebook-github-bot commented Nov 17, 2024

facebook-github-bot commented Nov 18, 2024

facebook-github-bot commented Nov 18, 2024

facebook-github-bot commented Nov 20, 2024

facebook-github-bot commented Nov 20, 2024

facebook-github-bot commented Nov 20, 2024

facebook-github-bot commented Nov 20, 2024

conbench-facebook bot commented Nov 21, 2024

netlify bot commented Oct 17, 2024 •

edited

Loading

gggrace14 left a comment •

edited

Loading

Yuhta Nov 7, 2024 •

edited

Loading