[Enhancement] Speed up JSON string parsing#75533
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a571a60a88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| *applied = true; |
There was a problem hiding this comment.
Validate the whole JSON before applying fast path
When the input has a constant path and malformed JSON after the requested value, such as {"k1":1,"bad":}, the on-demand parser can extract $.k1 without ever consuming the invalid tail; setting applied here then returns 1. The previous velocypack path fully parsed each row and would return NULL, or an error with allow_throw_exception, so invalid JSON can now produce non-null results.
Useful? React with 👍 / 👎.
| } else if (st.is_not_found()) { | ||
| result.append_null(); |
There was a problem hiding this comment.
Preserve lookups for escaped JSON keys
For constant paths, extract_from_object uses simdjson raw-key lookup, which does not unescape object keys; valid rows like {"\u006b1":1} with path $.k1 are reported as not found here and return NULL. The old velocypack path unescaped keys while parsing and returned the value, so this fast path changes results for valid JSON that uses escaped member names.
Useful? React with 👍 / 👎.
|
fyi #73279 |
- Replace std::iswspace in parse_json_or_string with an inline ASCII whitespace check: avoids UB when passing a (possibly negative) char to iswspace and removes locale overhead. Evidence: https://quick-bench.com/q/qp3OqCWLRKWA4heZS677TuLLOf8 - Add a reusable velocypack Parser overload of parse_json_or_string (make_reusable_parser) and use it in CAST(VARCHAR AS JSON) so the Parser/Builder buffer is allocated once per column instead of once per row (up to ~2.6x on small objects). - Add the BM_ParseJson benchmark (OneShot vs Reused) and fix build.sh --module getopt to accept an argument. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: zhangyifan27 <chinazhangyifan@163.com> Co-authored-by: Cursor <cursoragent@cursor.com>
a571a60 to
e5ce37c
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
Module-risk briefing for Codex reviewThis is review focus, not a finding list. Validate against the diff. High-confidence risks
|
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]❌ fail : 2 / 3 (66.67%) file detail
|
Only allocate the reusable vpack Parser for string/variant casts, and add a UT that exercises CAST(VARIANT AS JSON). Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: zhangyifan27 <chinazhangyifan@163.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Why I'm doing:
Speed up JSON string parsing
What I'm doing:
whitespace check: avoids UB when passing a (possibly negative) char to
iswspace and removes locale overhead.
Evidence: https://quick-bench.com/q/qp3OqCWLRKWA4heZS677TuLLOf8
(make_reusable_parser) and use it in CAST(VARCHAR AS JSON) so the
Parser/Builder buffer is allocated once per column instead of once per
row (up to ~2.6x on small objects).
--module getopt to accept an argument.
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: