sunchao commented on code in PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#discussion_r4052120944
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -246,68 +284,177 @@ fn parse_json_path(path: &str) -> Option<ParsedPath> {
}
}
- Some(ParsedPath {
- segments,
- has_wildcard,
- })
+ Some(ParsedPath { segments })
+}
+
+/// Jackson (and therefore Spark) rejects number tokens longer than 1000
+/// characters wherever they appear in the document — including values this
+/// evaluation skips — so `get_json_object` returns null. serde_json enforces
no
+/// such limit when skipping, so mirror it with a byte scan before parsing.
+fn has_oversized_number(json: &str) -> bool {
+ const MAX_NUMBER_LEN: usize = 1000;
+ let bytes = json.as_bytes();
+ let mut in_string = false;
+ let mut escaped = false;
+ let mut i = 0;
+ while i < bytes.len() {
+ let b = bytes[i];
+ if in_string {
+ if escaped {
+ escaped = false;
+ } else {
+ match b {
+ b'\\' => escaped = true,
+ b'"' => in_string = false,
+ _ => {}
+ }
+ }
+ i += 1;
+ continue;
+ }
+ if b == b'"' {
+ in_string = true;
+ } else if b.is_ascii_digit() || b == b'-' {
+ // A number token: digits, sign, decimal point and exponent marker
+ // all count towards Jackson's limit.
+ let start = i;
+ i += 1;
+ while i < bytes.len()
+ && matches!(bytes[i], b'0'..=b'9' | b'.' | b'e' | b'E' | b'+'
| b'-')
+ {
+ i += 1;
+ }
+ if i - start > MAX_NUMBER_LEN {
+ return true;
+ }
+ continue;
+ }
+ i += 1;
+ }
+ false
}
/// Evaluate a parsed JSONPath against a JSON string.
/// Returns the result as a string, or None if no match.
fn evaluate_path(json_str: &str, path: &ParsedPath) -> Option<String> {
- if !path.has_wildcard {
- return value_into_string(extract_no_wildcard(json_str,
&path.segments)?);
+ if has_oversized_number(json_str) {
+ return None;
}
Review Comment:
[P2] Avoid scanning every quoted-string byte before extraction
This unconditional pre-scan walks the entire input byte by byte, including
quoted strings, before the existing parser consumes it again. For the valid
document `{"a":1,"unused":"<64 KiB of x>"}` and path `$.a`, an optimized
benchmark using the exact evaluator sources measured median times of 10.066 us
on the base, 10.152 us on the prior head, and 117.018 us here, about 11.6x
slower than the base. The path was parsed once, inputs/results were
black-boxed, and revisions alternated across three rounds of 4,000 calls. A
separate harness reproduced the slowdown; removing only this scan in a
diagnostic copy restored the 64 KiB case to baseline. These are
evaluator-component timings, not whole-query timings.
Could you retain numeric validation while skipping quoted strings
efficiently, or integrate it into parsing, and add a representative benchmark?
Ordinary documents with large unselected string fields now pay this cost on
every extraction even when there is no numeric-length violation.
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -246,68 +284,177 @@ fn parse_json_path(path: &str) -> Option<ParsedPath> {
}
}
- Some(ParsedPath {
- segments,
- has_wildcard,
- })
+ Some(ParsedPath { segments })
+}
+
+/// Jackson (and therefore Spark) rejects number tokens longer than 1000
+/// characters wherever they appear in the document — including values this
+/// evaluation skips — so `get_json_object` returns null. serde_json enforces
no
+/// such limit when skipping, so mirror it with a byte scan before parsing.
+fn has_oversized_number(json: &str) -> bool {
+ const MAX_NUMBER_LEN: usize = 1000;
+ let bytes = json.as_bytes();
+ let mut in_string = false;
+ let mut escaped = false;
+ let mut i = 0;
+ while i < bytes.len() {
+ let b = bytes[i];
+ if in_string {
+ if escaped {
+ escaped = false;
+ } else {
+ match b {
+ b'\\' => escaped = true,
+ b'"' => in_string = false,
+ _ => {}
+ }
+ }
+ i += 1;
+ continue;
+ }
+ if b == b'"' {
+ in_string = true;
+ } else if b.is_ascii_digit() || b == b'-' {
+ // A number token: digits, sign, decimal point and exponent marker
+ // all count towards Jackson's limit.
+ let start = i;
+ i += 1;
+ while i < bytes.len()
+ && matches!(bytes[i], b'0'..=b'9' | b'.' | b'e' | b'E' | b'+'
| b'-')
+ {
+ i += 1;
+ }
+ if i - start > MAX_NUMBER_LEN {
+ return true;
Review Comment:
[P2] Count numeric length the way Jackson does
The new check counts all token bytes, but Jackson excludes the leading minus
sign from integer length and uses the integer/fraction/exponent digit counts
for floating-point length. For `{"a":1,"b":-<1000 consecutive nines>}` with
path `$.a`, Spark 4.0.4, Spark 4.1.3, the PR base, and the prior head return
`1`; this head returns SQL NULL because it counts 1,001 bytes. A finite decimal
also regresses: `[{"a":1,"b":0.<999 consecutive ones>}]` with `$[*].a` returns
`1` on those versions but SQL NULL here because the decimal point is counted.
Both new-head outputs also reproduce through the compiled scalar and both
column entry points.
Could you mirror Jackson's numeric length counters instead of the raw
token-byte length, and add signed, fractional, and exponent boundary cases?
Otherwise an unrelated, valid numeric field can null out an otherwise
successful extraction.
--
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]
