Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
/review-light |
Contributor
There was a problem hiding this comment.
Static review result: no blocking correctness issue found at exact head ebb82ff6de08a3b9c125f3fd692b649513b1bd31.
Critical checkpoint conclusions:
- Goal and proof: the change stops treating simdjson
NUMBER_ERROR/BIGINT_ERRORas unconditional success, validates fallback tokens against the exact JSON-number grammar, preserves representable large integers, and rejects non-finite conversion. The added BE unit cases and SQL regression cover malformed root/nested tokens, trailing content, uint64/int128 boundaries, long roots, overflow, underflow, constant inputs, casts, error modes, andjson_valid. - Scope and clarity: the implementation is focused in the shared
JsonbParserpath, with direct boundary tests and no unrelated production changes. - Concurrency: not applicable. Parser, padded input, token views, and writer state are invocation-local; no shared mutable state or locking is introduced.
- Lifecycle/static initialization: token views remain within the lifetime of the local padded input and are consumed synchronously. No static/global lifecycle or cross-TU initialization is added.
- Configuration: no configuration item is added or changed.
- Compatibility: this intentionally tightens acceptance of malformed or non-representable JSON text. Existing finite numbers and large-integer handling are preserved, and no JSONB physical format, symbol, protocol, or rolling-upgrade contract changes.
- Parallel paths:
JsonBinaryValue::from_json_stringis the common ingress used by the SQL parse/error variants,json_valid, string-to-JSON casts, literals/constant folding, JSON SerDe, and relevant load/connector paths. The unrelated DOM/Variant parsers are not bypasses for this JSONB text contract. - Conditional checks: the root end-pointer check is necessary because simdjson 3.11.6 returns the numeric error before checking trailing content for these cases; nested iteration still validates the following comma/closing delimiter. The grammar, whitespace trim, int128 result check, and finite-double gate each correspond to a concrete input class.
- Test coverage: the new unit and regression coverage is broad across positive, negative, root, nested, constant, and vectorized paths; existing ordinary-number tests continue to cover the normal success path.
- Expected results: the new
.outrows are internally consistent, deterministic (order_qtplusORDER BY), and follow the repository's error-test conventions. I did not independently regenerate them. - Observability: returned errors identify the simdjson error and offending token; this local parsing change does not warrant new logs or metrics.
- Transactions/persistence/data writes: no transaction, EditLog, publish, storage-version, delete-bitmap, or write-atomicity mechanism is changed. Load behavior changes only through the shared validation result.
- FE/BE variables: no new cross-process or FE/BE field is introduced.
- Performance: ordinary successful parsing avoids the old zero-value reparse. Extra grammar/conversion passes occur only on simdjson numeric-error fallback paths and remain linear; no material hot-path regression was found.
- Other review points: header hygiene passed, and current CI reports the formatter/style gates passing. No additional correctness, memory-safety, error-propagation, portability, or material performance issue was substantiated.
User focus (/review-light, represented as -light in the bundle): no additional focus-specific issue was found; the full required review was still completed.
Validation scope: static review only. Per the runner instruction, I did not build Doris or execute the author-listed BE unit/regression tests, so their claimed execution is not independently verified here.
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27456 ms |
Member
Author
|
run cloud_p0 |
Contributor
TPC-DS: Total hot run time: 151924 ms |
Contributor
ClickBench: Total hot run time: 23.89 s |
…g them
### What problem does this PR solve?
Issue Number: None
Problem Summary:
`JSON_PARSE`, `JSON_PARSE_ERROR_TO_NULL`, `JSON_PARSE_ERROR_TO_VALUE`,
`CAST(... AS JSON)`, JSON column loads and every other path that goes through
`JsonbParser` accepted invalid JSON numbers such as `01`, `00`, `-01`, `1.`,
`01.5` or `1.e5` and silently turned them into a different valid value: `01`
became `0`, `1.` became `1`, `1.e5` became `100000`, and `1e400` became `inf`.
This happened at the top level as well as inside arrays and objects, so
malformed source data was normalized instead of rejected.
Root cause: `JsonbParser::parse_number_success` treated simdjson's
`NUMBER_ERROR` as success in order to support integers just above the uint64
range (simdjson reports `18446744073709551616` as `NUMBER_ERROR` and longer
integers as `BIGINT_ERROR`). However, simdjson also returns `NUMBER_ERROR` for
malformed tokens (leading zeros, a trailing `.`, an incomplete exponent,
trailing garbage) and for values beyond the double range. `simdjson_result::get`
leaves the caller's `simdjson::ondemand::number` untouched on error, so the
writer emitted its zero-initialized payload for integer-looking tokens; for
float-looking tokens the `number == 0` branch re-parsed the raw token with
`StringParser::string_to_float`, which accepts `1.`, `01.5` and `1e400`.
Fix: `write_number` only falls back to the raw token for `NUMBER_ERROR` and
`BIGINT_ERROR`; any other error is returned as `InvalidArgument`. The raw token is validated against the JSON number grammar
`-?(0|[1-9][0-9]*)(\.[0-9]+)?([eE][+-]?[0-9]+)?` (after trimming the trailing
JSON whitespace that `raw_json_token()` may include); a digits-only token is
then parsed as int128, anything else as a finite double. A root number must
additionally reach the end of the document, because simdjson reports
`NUMBER_ERROR` for `18446744073709551616 0` before it checks for trailing
content and the raw token stops at the first token. Malformed input now fails
with `InvalidArgument`, so `JSON_PARSE` raises an error and
`JSON_PARSE_ERROR_TO_NULL` returns NULL. The successful path takes the number
type from the parsed `number` object and no longer re-parses zero values with
`StringParser`, since a value simdjson parsed successfully is already exact.
Before:
```
SELECT json_parse('01'), json_parse('[1.]'), json_parse('{"k":1.e5}'), json_parse('1e400');
-- 0 [1] {"k":100000} inf
```
After:
```
SELECT json_parse('01');
-- ERROR: Parse json document failed at row 0, error: [INVALID_ARGUMENT]simdjson get_number failed: NUMBER_ERROR: Problem while parsing a number, raw string is: 01
SELECT json_parse_error_to_null('01');
-- NULL
```
Valid large numbers keep working: `18446744073709551616` and
`-9223372036854775809` are still stored as int128, and integers beyond int128
still fall back to double. As a side effect, a root number longer than
simdjson's root scalar buffer (1082 bytes) that contains a fraction or an
exponent is now accepted through the same raw-token path instead of failing;
the nested form of such numbers was already accepted.
### Release note
JSON text parsing (`JSON_PARSE` and its variants, `CAST(... AS JSON)`,
`JSON_VALID`, and loading into JSON columns) now rejects malformed JSON number
tokens such as `01`, `00`, `1.` or `1.e5`, and numbers beyond the double range
such as `1e400`, instead of silently converting them to a different value.
### Check List (For Author)
- Test:
- Unit Test: `JsonbParserTest.*` in `be/test/util/jsonb_parser_simd_test.cpp`
- Regression test: `datatype_p0/json/test_json_parse_invalid_number`
- Regression test: `external_table_p0/dialect_compatible/sql/presto/scalar/TestJsonFunctions`
expected output updated: `json_array_contains('[9.6E400]', 4.2)` now yields NULL
because `9.6E400` is beyond the double range and is rejected
- Behavior changed: Yes (invalid JSON number tokens and numbers beyond the double range are rejected on every JSON text parsing path; previously they were normalized to a different value)
- Does this need documentation: No
mrhhsg
force-pushed
the
fix/json-parse-reject-malformed-numbers
branch
from
September 24, 2026 14:19
ebb82ff to
852c4aa
Compare
Member
Author
|
/review-light |
Contributor
There was a problem hiding this comment.
Static review outcome: request changes for one P2 resource-amplification issue in the newly added malformed-number diagnostics.
Critical checkpoint conclusions:
- Goal: the parser change otherwise implements strict JSON-number grammar, preserves supported int128/finite-double values, and rejects malformed, trailing, and non-finite inputs. The added unit and regression cases exercise the core behavior.
- Scope: the production change is focused on number parsing; the accompanying unit/regression and Presto expected-output updates match that scope.
- Concurrency: parsing uses invocation-local parser/writer state and adds no threads, shared mutable state, locks, or atomic-ordering concern.
- Lifecycle/static initialization: no nontrivial ownership, cleanup, static initialization, or cross-TU lifecycle is introduced.
- Configuration: no configuration item or dynamic-reload behavior is added.
- Compatibility: JSONB storage/protocol layout and function symbols are unchanged. Mixed-version nodes may temporarily differ on acceptance of malformed numeric text, which is the intended behavior change.
- Parallel paths: JSON_PARSE variants, JSON_VALID, strict/non-strict string-to-JSON casts, and text/SerDe/load conversions converge on
JsonBinaryValue::from_json_string; no alternate text parser requiring the same fix was found. - Conditions/error handling: the root end check and grammar/fallback branches are explained and statuses are propagated or deliberately converted to fail/NULL/default/false. The one remaining error-path problem is the unbounded diagnostic called out inline.
- Tests: coverage includes root/nested malformed forms, trailing content, whitespace, integer boundaries, long scalars, overflow, underflow, constants, vectors, and tolerant modes. Explicit strict-cast/load cases would strengthen coverage but share the same parser path and did not reveal a separate defect.
- Expected results: the new outputs are internally consistent, including the Presto overflow case becoming NULL. This was a static-only review; I did not run builds or tests, so author/CI results were not independently executed here.
- Observability: no new metrics or logs are needed for this local parser change; diagnostics should remain useful but bounded.
- Transactions/persistence/data writes: no EditLog, transaction, publication, atomic-write, crash-recovery, or persistent-format logic changes.
- FE/BE variables: no new cross-process field or sending path is introduced.
- Performance/memory: the accepted P2 finding is a new O(input)-sized error allocation on tolerant paths that discard the message. No other CPU, allocation, or memory-safety issue was substantiated.
- Other issues/user focus:
-lightsupplied no additional targeted concern. Two full review rounds converged with all candidates resolved and no additional valuable finding.
Validation: static inspection only, as required by the review harness; no build or test was run.
…rors ### What problem does this PR solve? Issue Number: None Related PR: apache#68484 Problem Summary: The malformed-number diagnostics added to `JsonbParser` formatted the whole raw token into the returned `Status`. The token can be as long as the input, and tolerant callers such as `JSON_VALID`, `JSON_PARSE_ERROR_TO_NULL` and `JSON_PARSE_ERROR_TO_VALUE` discard the message immediately, so rejecting a multi-megabyte malformed number allocated another input-sized string on top of the source column and simdjson's padded copy. `quote_token` now quotes at most 64 bytes of the token and reports its full length instead; every number diagnostic, including the root trailing-content check, goes through it. ### Release note None ### Check List (For Author) - Test: - Unit Test: `JsonbParserTest.ParseJsonMalformedNumberErrorMessageIsBounded` in `be/test/util/jsonb_parser_simd_test.cpp` - Regression test: `datatype_p0/json/test_json_parse_invalid_number` (unchanged, error text still matches) - Behavior changed: No (only the quoted part of the error message is bounded) - Does this need documentation: No
Member
Author
|
/review-light |
Contributor
|
Codex automated review failed and did not complete. Error: Codex exited with status 0 without a terminal turn event; review is incomplete Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Member
Author
|
/review-light |
Contributor
|
Codex automated review failed and did not complete. Error: Codex exited with status 0 without a terminal turn event; review is incomplete Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Member
Author
|
/review-light |
Contributor
There was a problem hiding this comment.
Static review outcome: request changes for one P2 resource-amplification issue in the CAST-to-JSONB error path.
Critical checkpoint conclusions:
- Goal and correctness: the parser now validates the JSON number grammar, preserves supported int128 and finite-double values, rejects malformed/trailing/non-finite numbers, and keeps root/nested behavior aligned.
- Scope: the production change is focused in the shared JsonbParser path; the unit and regression updates exercise malformed root/nested forms, boundaries, overflow/underflow, constants, casts, tolerant modes, and JSON_VALID.
- Concurrency/lifecycle/configuration: parser and writer state are invocation-local; no locks, shared mutable state, static initialization, or configuration behavior is changed.
- Compatibility/parallel paths: JSONB storage/protocol contracts are unchanged, and the inspected JSON_PARSE, JSON_VALID, error-tolerant, constant, SerDe/load, and CAST paths converge on the reviewed parser.
- Error and memory behavior: parser-side raw-token diagnostics are bounded, but the CAST wrapper still materializes the full source on every parse failure; the inline comment calls out this distinct downstream hole.
- Test/validation status: expected outputs and coverage are internally consistent, but this is static-only review. I did not build Doris or execute the author-listed tests, as prohibited by the review harness.
- User focus (
-light): no additional focus-specific issue was found.
The existing parser diagnostic thread was treated as a hard duplicate fence; the inline issue below is specifically the separate CAST caller allocation.
### What problem does this PR solve? Issue Number: None Related PR: apache#68484 Problem Summary: `ParseJsonbFromString::parse_json` formatted the whole source string into the returned `Status` whenever parsing failed. For a multi-megabyte malformed value both strict and non-strict CAST to JSONB allocated and copied another input-sized string, and non-strict CAST discards the status right away. The bounding helper used by `JsonbParser` number diagnostics is now exposed as `JsonbParser::bounded_quote`, and the CAST wrapper quotes the source through it: at most 64 bytes plus the full length. ### Release note None ### Check List (For Author) - Test: - Unit Test: `FunctionCastTest.test_strict_cast_string_to_jsonb_*` in `be/test/exprs/function/cast/cast_to_jsonb_test.cpp`, `JsonbParserTest.*` - Regression test: `function_p0/cast/test_try_cast`, `datatype_p0/json/test_json_parse_invalid_number` - Behavior changed: No (only the quoted part of the error message is bounded) - Does this need documentation: No
Member
Author
|
/review-light |
Contributor
There was a problem hiding this comment.
Static review result: no new blocking correctness issue found at exact head 1a1da27f6bde54a8649ed1763a37e07fc5926b2c. The two earlier P2 diagnostic-allocation issues are fixed at this head, so no duplicate inline comments are being submitted.
Critical checkpoint conclusions:
- Goal and proof: the shared JSONB parser now rejects malformed JSON-number grammar and non-finite conversions while preserving exact int128 values and finite-double fallback. The added parser unit cases and SQL regression cover malformed root/nested tokens, trailing content, uint64/int128 boundaries, long roots, overflow, underflow, constants, casts, tolerant modes, and
json_valid; the CAST unit test covers short and 1 MiB bounded diagnostics. - Scope and clarity: the implementation is focused in
JsonbParserand the string-to-JSONB CAST error wrapper, with corresponding unit/regression and expected-output updates. No unrelated production behavior is changed. - Concurrency: not applicable. The parser, padded input, token views, number state, and writer are invocation-local; no shared state, threads, locks, or atomics are introduced.
- Lifecycle/static initialization: raw-token views are consumed synchronously while the local padded input is alive, and reusable writer state is reset before each parse. No static/global lifetime or cross-TU initialization is added.
- Configuration: no configuration item or dynamic-reload behavior is added.
- Compatibility: the intended mixed-version difference is acceptance versus rejection of invalid numeric text. Accepted values retain the existing JSONB writer types; there is no storage layout, function symbol, wire protocol, or FE/BE contract change.
- Parallel paths:
JsonBinaryValue::from_json_stringis the common ingress forjson_parsemodes,json_valid, strict/non-strict string CAST, JSON SerDe/load conversion, literals, and located connectors. Callers consistently propagate failure or intentionally map it to NULL/default/false. - Conditional checks: simdjson 3.11.6 can return
NUMBER_ERROR/BIGINT_ERRORbefore root trailing-content validation, so the raw-token end check is necessary. Its token extent includes legal trailing JSON whitespace through EOF and ends before a following token; nested iteration still validates commas and closing delimiters. The exact grammar, int128 parse-result check, and finite-double gate each address a concrete input class. - Test coverage: negative and positive root, array, object, constant, and vectorized paths are covered, including bounded multi-megabyte errors. A dedicated load regression is not added, but the inspected load SerDe uses the same parser boundary. No missing test exposed a distinct correctness defect.
- Expected results: the new result file is deterministic and consistent with the suite. The Presto change from
falseto NULL corresponds to parsing invalid[9.6E400]. Result files were not independently regenerated in this runner. - Observability: bounded errors retain the simdjson error, offending prefix, and full byte length. This local validation change does not warrant new logs or metrics.
- Transactions/persistence/data writes: no transaction, EditLog, publish, storage-version, delete-bitmap, crash-recovery, or atomic-write mechanism is modified.
- FE/BE variables: no cross-process field or additional sending path is introduced.
- Performance and memory: ordinary successful numbers remain on simdjson's fast path. Recovery performs linear, allocation-free token scans only on numeric-error/oversized-number paths, and all newly relevant diagnostics are bounded. No material hot-path or memory-safety issue was substantiated.
- Other review points: the success-only
big_integerarm is unreachable under the pinned simdjsonget_number()contract, header/build hygiene passed, and the full changed-file sweep left no unresolved candidate.
User focus (-light): no additional focus-specific concern was supplied or found; the full required review was still completed.
Validation scope: static review only. Per the runner instruction, I did not build Doris or execute the author-listed BE unit/regression tests. The author's reported ASAN/unit/regression runs were considered but are not independently verified here.
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27939 ms |
Contributor
TPC-DS: Total hot run time: 152445 ms |
Contributor
ClickBench: Total hot run time: 24.08 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Problem Summary:
JSON_PARSE,JSON_PARSE_ERROR_TO_NULL,JSON_PARSE_ERROR_TO_VALUE,CAST(... AS JSON), JSON column loads and every other path that goes throughJsonbParseraccepted invalid JSON numbers such as01,00,-01,1.,01.5or1.e5and silently turned them into a different valid value:01became
0,1.became1,1.e5became100000, and1e400becameinf.This happened at the top level as well as inside arrays and objects, so
malformed source data was normalized instead of rejected.
Root cause:
JsonbParser::parse_number_successtreated simdjson'sNUMBER_ERRORas success in order to support integers just above the uint64range (simdjson reports
18446744073709551616asNUMBER_ERRORand longerintegers as
BIGINT_ERROR). However, simdjson also returnsNUMBER_ERRORformalformed tokens (leading zeros, a trailing
., an incomplete exponent,trailing garbage) and for values beyond the double range.
simdjson_result::getleaves the caller's
simdjson::ondemand::numberuntouched on error, so thewriter emitted its zero-initialized payload for integer-looking tokens; for
float-looking tokens the
number == 0branch re-parsed the raw token withStringParser::string_to_float, which accepts1.,01.5and1e400.Fix:
write_numberonly falls back to the raw token forNUMBER_ERRORandBIGINT_ERROR; any other error is returned asInvalidArgument. The raw token is validated against the JSON number grammar-?(0|[1-9][0-9]*)(\.[0-9]+)?([eE][+-]?[0-9]+)?(after trimming the trailingJSON whitespace that
raw_json_token()may include); a digits-only token isthen parsed as int128, anything else as a finite double. A root number must
additionally reach the end of the document, because simdjson reports
NUMBER_ERRORfor18446744073709551616 0before it checks for trailingcontent and the raw token stops at the first token. Malformed input now fails
with
InvalidArgument, soJSON_PARSEraises an error andJSON_PARSE_ERROR_TO_NULLreturns NULL. The successful path takes the numbertype from the parsed
numberobject and no longer re-parses zero values withStringParser, since a value simdjson parsed successfully is already exact.Before:
After:
Valid large numbers keep working:
18446744073709551616and-9223372036854775809are still stored as int128, and integers beyond int128still fall back to double. As a side effect, a root number longer than
simdjson's root scalar buffer (1082 bytes) that contains a fraction or an
exponent is now accepted through the same raw-token path instead of failing;
the nested form of such numbers was already accepted.
Release note
JSON text parsing (
JSON_PARSEand its variants,CAST(... AS JSON),JSON_VALID, and loading into JSON columns) now rejects malformed JSON numbertokens such as
01,00,1.or1.e5, and numbers beyond the double rangesuch as
1e400, instead of silently converting them to a different value.Check List (For Author)
JsonbParserTest.*inbe/test/util/jsonb_parser_simd_test.cppdatatype_p0/json/test_json_parse_invalid_number