mrhhsg commented on code in PR #68484:
URL: https://github.com/apache/doris/pull/68484#discussion_r4103128954
##########
be/src/util/jsonb_parser_simd.h:
##########
@@ -244,38 +233,139 @@ struct JsonbParser {
return Status::OK();
}
- static Status write_number(simdjson::ondemand::number num,
- simdjson ::ondemand::number_type num_type,
+ // raw_json_token() spans up to the start of the next token, so it may end
with JSON
+ // whitespace that is not part of the number.
+ static std::string_view trim_trailing_whitespace(std::string_view token) {
+ while (!token.empty() && (token.back() == ' ' || token.back() == '\t'
||
+ token.back() == '\n' || token.back() ==
'\r')) {
+ token.remove_suffix(1);
+ }
+ return token;
+ }
+
+ // Error messages quote the offending token so that the bad value can be
located, but
+ // the token is as long as the input (a malformed row may be a
multi-megabyte digit run)
+ // and tolerant callers such as json_valid or the error-to-null variants
discard the
+ // message right away. Keep the quoted part bounded and report the full
length instead.
+ static std::string quote_token(std::string_view token) {
+ constexpr size_t kMaxQuotedTokenLen = 64;
+ token = trim_trailing_whitespace(token);
+ if (token.size() <= kMaxQuotedTokenLen) {
+ return std::string(token);
+ }
+ return fmt::format("{}... (truncated, {} bytes)", token.substr(0,
kMaxQuotedTokenLen),
+ token.size());
+ }
+
+ // Matches the JSON number grammar exactly:
+ // -?(0|[1-9][0-9]*)(\.[0-9]+)?([eE][+-]?[0-9]+)?
+ static bool is_json_number(std::string_view token) {
+ size_t i = 0;
+ const size_t n = token.size();
+ auto skip_digits = [&]() {
+ const size_t start = i;
+ while (i < n && token[i] >= '0' && token[i] <= '9') {
+ ++i;
+ }
+ return i > start;
+ };
+ if (i < n && token[i] == '-') {
+ ++i;
+ }
+ if (i < n && token[i] == '0') {
+ ++i;
+ } else if (!skip_digits()) {
+ return false;
+ }
+ if (i < n && token[i] == '.') {
+ ++i;
+ if (!skip_digits()) {
+ return false;
+ }
+ }
+ if (i < n && (token[i] == 'e' || token[i] == 'E')) {
+ ++i;
+ if (i < n && (token[i] == '+' || token[i] == '-')) {
+ ++i;
+ }
+ if (!skip_digits()) {
+ return false;
+ }
+ }
+ return i == n;
+ }
+
+ // According to https://github.com/simdjson/simdjson/pull/2139, integers
that do not fit
+ // in 64 bits can be handled by parsing the raw_json_token ourselves:
simdjson returns
+ // NUMBER_ERROR for 18446744073709551616 (one above uint64 max) and
BIGINT_ERROR for
+ // longer integers such as 18446744073709551616231231.
+ // However NUMBER_ERROR is also what simdjson returns for malformed tokens
(leading
+ // zeros like 01, a trailing dot like 1., 1e, trailing garbage like 1x)
and for values
+ // beyond the double range. `num` carries nothing usable in any of these
cases, so the
+ // raw token is first checked against the JSON number grammar and then
parsed as int128
+ // or double.
+ static Status write_number_from_token(simdjson::error_code res,
std::string_view raw_string,
+ JsonbWriter& writer) {
+ std::string_view token = trim_trailing_whitespace(raw_string);
+ if (!is_json_number(token)) {
+ return Status::InvalidArgument("simdjson get_number failed: {},
raw string is: {}",
+ simdjson::error_message(res),
quote_token(token));
+ }
+
+ // StringParser::string_to_int silently truncates a fraction, so only
a token made of
+ // digits may be parsed as an integer.
+ if (token.find_first_of(".eE") == std::string_view::npos) {
+ StringParser::ParseResult result;
+ auto val = StringParser::string_to_int<int128_t>(token.data(),
token.size(), &result);
+ if (result == StringParser::PARSE_SUCCESS) {
+ if (!writer.writeInt128(val)) {
+ return Status::InvalidArgument("writeInt128 failed");
+ }
+ return Status::OK();
+ }
+ }
+
+ // Either a floating point number or an integer beyond int128.
Converting it to double
+ // may lose precision, but for JSON, exchanging data as plain text
between different
+ // systems may inherently cause precision loss.
+ StringParser::ParseResult result;
+ double double_val =
+ StringParser::string_to_float<double>(token.data(),
token.size(), &result);
+ if (result != StringParser::PARSE_SUCCESS ||
!std::isfinite(double_val)) {
+ return Status::InvalidArgument("invalid number, raw string is:
{}", quote_token(token));
+ }
+ if (!writer.writeDouble(double_val)) {
+ return Status::InvalidArgument("writeDouble failed");
+ }
+ return Status::OK();
+ }
+
+ static Status write_number(simdjson::error_code res,
simdjson::ondemand::number num,
std::string_view raw_string, JsonbWriter&
writer) {
- // The simdjson library supports four types of numbers:
+ switch (res) {
+ case simdjson::error_code::SUCCESS:
Review Comment:
Fixed in 1a1da27f6bd.
- Moved the 64-byte bound out of `quote_token()` into a public
`JsonbParser::bounded_quote()`. `quote_token()` now just calls
`bounded_quote(trim_trailing_whitespace(token))`, so the number diagnostics
produce exactly the same text as before.
- `ParseJsonbFromString::parse_json` (`cast_to_jsonb.h`) now quotes the
source with `JsonbParser::bounded_quote(str.to_string_view())` instead of
`str.to_string()`. A long source shows only its first 64 bytes, followed by
`... (truncated, N bytes)`. Sources of 64 bytes or less are still quoted in
full. Strict and non-strict CAST both go through `parse_json`, so this fixes
both modes.
- Added a new UT file, `be/test/exprs/function/cast/cast_to_jsonb_test.cpp`.
It runs the real String -> JSONB cast wrapper in strict mode with two inputs:
- a short malformed source, which is still quoted in full;
- a 1 MiB malformed source, whose message must stay under 512 bytes and
contain the 64-byte prefix and the full length.
Validation:
- `FunctionCastTest.test_strict_cast_string_to_jsonb*`, `JsonbParserTest.*`
and `FunctionJsonbTEST.*` pass (87 tests, ASAN).
- On a local cluster, `function_p0/cast/test_try_cast` and
`datatype_p0/json/test_json_parse_invalid_number` pass.
- Also checked by hand on that cluster:
- Strict `CAST` of a 100 KB malformed value returns an error of about 380
bytes. Both the source and the parser token are quoted with `(truncated, ...)`.
- Non-strict `CAST` of the same value returns NULL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
