You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "u70b3 (via GitHub)" <gi...@apache.org> on 2026/07/18 00:32:07 UTC
[PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
u70b3 opened a new pull request, #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971
## Which issue does this PR close?
Closes #4947.
## Rationale for this change
For a JSON object with duplicate keys, Spark's `GetJsonObjectEvaluator` stops at the first matching field, but Comet's native implementation resolved the key to its *last* occurrence (matching `serde_json`'s overwrite semantics rather than Spark's):
```sql
SELECT get_json_object('{"a":1,"a":2}', '$.a');
-- Spark: 1
-- Comet (before this PR): 2
```
## What changes are included in this PR?
`SegmentVisitor::visit_map` in `native/spark-expr/src/string_funcs/get_json_object.rs` now locks in the first key match and consumes later occurrences via `IgnoredAny` instead of re-parsing them with `PathSeed`:
- The lock is keyed on the *key match*, not on a successful subpath resolution: if the first matching value does not contain the rest of the path, the result is `null` and the second duplicate key is never consulted, matching Spark.
- All entries are still visited, so the existing malformed/trailing-garbage rejection behavior is unchanged.
## How are these changes tested?
- Flipped the existing `test_duplicate_key_last_wins` unit test to `test_duplicate_key_first_wins`.
- Added `test_duplicate_key_first_wins_nested` (`{"a":{"b":1},"a":{"b":2}}` / `$.a.b` -> `1`).
- Added `test_duplicate_key_first_match_locks_value` (`{"a":{"x":1},"a":{"b":2}}` / `$.a.b` -> `null`, matching Spark's lock-on-first-match semantics).
- Full `datafusion-comet-spark-expr` suite passes (497 tests), plus `clippy` and `fmt` checks.
- Checked `spark/src/test/resources/sql-tests/` and `CometJsonJvmSuite` for duplicate-key cases: the only repeated keys there are across *different objects inside arrays*, which are unaffected by this change.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on code in PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#discussion_r4051935236
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -372,25 +435,65 @@ impl<'de> Visitor<'de> for SegmentVisitor<'_> {
}
fn visit_seq<A: SeqAccess<'de>>(self, mut seq: A) -> Result<Self::Value, A::Error> {
- let PathSegment::Index(idx) = &self.segments[0] else {
- IgnoredAny.visit_seq(seq)?;
- return Ok(None);
- };
-
- for _ in 0..*idx {
- if seq.next_element::<IgnoredAny>()?.is_none() {
- return Ok(None);
+ match &self.segments[0] {
+ PathSegment::Index(idx) => {
+ for _ in 0..*idx {
+ if seq.next_element::<IgnoredAny>()?.is_none() {
+ return Ok(PathResult::default());
+ }
+ }
+ let found = seq
+ .next_element_seed(PathSeed {
+ segments: &self.segments[1..],
+ reject_direct_null: false,
+ flatten: self.flatten,
+ })?
+ .unwrap_or_default();
+ // The remaining elements are still visited, so that a malformed element
+ // after the match yields no match, as a full parse would.
+ IgnoredAny.visit_seq(seq)?;
+ Ok(found)
+ }
+ PathSegment::Wildcard => {
+ let mut found = PathResult::default();
+ while let Some(mut result) = seq.next_element_seed(PathSeed {
+ segments: &self.segments[1..],
+ reject_direct_null: false,
+ flatten: self.flatten,
+ })? {
+ if result.matched {
+ found.matched = true;
+ found.values.append(&mut result.values);
Review Comment:
Fixed in 7099e9d2 — thank you for the precise diagnosis; the QuotedStyle transition on `[0]` followed by `[*]` was the key.
Rather than patching the single case, I ported Spark's write styles end to end: `Style {Raw, Quoted, Flatten}` now propagates through the evaluation the way `evaluatePath` threads its `style` parameter, and each wildcard arm makes its own wrapper decision — Quoted style always keeps the wrapper, Raw/Flatten buffer the element writes and strip the outer brackets only for a lone writer. Results are modeled on Spark's generator protocol (a list of fragment writes plus the dirty flag), which turned out to matter beyond this case: the Quoted and double-wildcard arms write their brackets even when nothing inside matched, and Spark's generator keeps those bytes, so `{"a":[[{}]],"a":[[{"b":1}]]}` with `$.a[0][*].b` really produces `[] [1]` on 4.1.3 (root-level writes separated by a space). That is reproduced as well.
`[[[[[[[1]]]]]]]` / `$[0][*][0][*][*]` now returns `[[1]]`, and the case is in `get_json_object.sql` together with the simpler `$[0][*]` shapes.
This also closes the two gaps recorded earlier in the thread: `$.store.basket[0][*].b` → `["y"]` and `$.a[*].b[*]` → `[[1,2],[3]]`, both now asserted positively.
Validation: a 600-case differential (20 documents × 30 paths) against Spark 4.1.3's `GetJsonObjectEvaluator` driven directly from the 4.1.3 catalyst jar; 596/600 match. The 4 mismatches are all `$`-on-duplicate-keys documents — serde_json's `Value` materialization collapses duplicate keys where Spark's token copy preserves them. That gap predates this PR (the pre-change UDF materializes `Value` the same way) and is unchanged by it.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#discussion_r4043529697
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -372,25 +435,65 @@ impl<'de> Visitor<'de> for SegmentVisitor<'_> {
}
fn visit_seq<A: SeqAccess<'de>>(self, mut seq: A) -> Result<Self::Value, A::Error> {
- let PathSegment::Index(idx) = &self.segments[0] else {
- IgnoredAny.visit_seq(seq)?;
- return Ok(None);
- };
-
- for _ in 0..*idx {
- if seq.next_element::<IgnoredAny>()?.is_none() {
- return Ok(None);
+ match &self.segments[0] {
+ PathSegment::Index(idx) => {
+ for _ in 0..*idx {
+ if seq.next_element::<IgnoredAny>()?.is_none() {
+ return Ok(PathResult::default());
+ }
+ }
+ let found = seq
+ .next_element_seed(PathSeed {
+ segments: &self.segments[1..],
+ reject_direct_null: false,
+ flatten: self.flatten,
+ })?
+ .unwrap_or_default();
+ // The remaining elements are still visited, so that a malformed element
+ // after the match yields no match, as a full parse would.
+ IgnoredAny.visit_seq(seq)?;
+ Ok(found)
+ }
+ PathSegment::Wildcard => {
+ let mut found = PathResult::default();
+ while let Some(mut result) = seq.next_element_seed(PathSeed {
+ segments: &self.segments[1..],
+ reject_direct_null: false,
+ flatten: self.flatten,
+ })? {
+ if result.matched {
+ found.matched = true;
+ found.values.append(&mut result.values);
Review Comment:
[P2] Preserve the quoted wrapper around nested wildcard results
For input `[[[[[[[1]]]]]]]` and path `$[0][*][0][*][*]`, Spark 4.1.3 and the PR base return `[[1]]`, but this head returns `[1]`. I also reproduced `[1]` through the compiled native scalar and both column entry points.
The first `[0]` followed by `[*]` makes Spark enter `QuotedStyle`, so that surrounding wildcard must retain its array wrapper even when only one child matches. The new double wildcard flattens the selected subtree, but this append and the final singleton serialization do not preserve the surrounding quoted wrapper. Although other nested-wildcard formatting gaps predate this PR, this particular input is correct on the base and regresses here.
Could you preserve Spark's per-level raw/quoted/flatten output styles, including the index-before-wildcard transition, and add this regression case to the SQL tests?
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -255,59 +276,97 @@ fn parse_json_path(path: &str) -> Option<ParsedPath> {
/// 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)?);
+ let mut result = extract_path(json_str, &path.segments)?;
Review Comment:
[P2] Preserve validation of oversized numeric tokens in skipped wildcard fields
For a document of the form `[{"a":1,"b":<1001 consecutive nines>}]` and path `$[*].a`, Spark 4.1.3 and the PR base return SQL NULL, while this head returns `1`. Reversing the field order gives the same result. I reproduced this through the compiled native scalar and both column entry points.
Routing wildcard paths through `extract_path` makes the unselected numeric value go through `IgnoredAny`, which does not enforce Spark's numeric-token length constraint. Spark accepts the skipped 1000-digit token but rejects the 1001-digit token. The old wildcard parser also rejected the latter value, so this is a new wildcard regression even though an analogous non-wildcard compatibility gap already existed.
Could you preserve validation of skipped numeric tokens and add coverage on both sides of Spark's 1000-digit boundary?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "rich7420 (via GitHub)" <gi...@apache.org>.
rich7420 commented on code in PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#discussion_r4059775926
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -246,68 +284,236 @@ fn parse_json_path(path: &str) -> Option<ParsedPath> {
}
}
- Some(ParsedPath {
- segments,
- has_wildcard,
- })
+ Some(ParsedPath { segments })
+}
+
+/// Jackson (and therefore Spark) rejects numbers whose digit count exceeds
+/// 1000 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 (see its `ignore_integer`/`ignore_decimal`), so mirror
+/// Jackson's `StreamReadConstraints` counters with a byte scan before parsing:
+/// the sign and the decimal point do not count, integers are limited by their
+/// digit count, and floats by the sum of their integer-part (a lone leading
+/// zero counts as zero digits), fraction and exponent digit counts.
+/// Find the end of a string body starting at `i` (just past the opening
+/// quote). Short bodies are scanned inline; long bodies use memchr2, the same
+/// approach as serde_json's `ignore_str`. Returns the index just past the
+/// closing quote, or None for an unterminated string (the parser rejects the
+/// document anyway).
+#[inline]
+fn skip_string_body(bytes: &[u8], mut i: usize) -> Option<usize> {
+ const SHORT_STRING: usize = 32;
+ if bytes.len() - i <= SHORT_STRING {
+ while i < bytes.len() {
+ match bytes[i] {
+ b'"' => return Some(i + 1),
+ b'\\' => i += 2,
+ _ => i += 1,
+ }
+ }
+ return None;
+ }
+ loop {
+ match memchr::memchr2(b'"', b'\\', &bytes[i..]) {
+ Some(off) if bytes[i + off] == b'"' => return Some(i + off + 1),
+ Some(off) => i += off + 2, // escaped byte
Review Comment:
A truncated JSON string can now abort the native query instead of returning NULL. For `{"a":1,"unused":"` followed by 40 `x` characters and a final backslash, `i` advances past the buffer and panics. I reproduced this with constant and column paths. Please bounds-check the escape skip and add a regression.
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -246,68 +284,236 @@ fn parse_json_path(path: &str) -> Option<ParsedPath> {
}
}
- Some(ParsedPath {
- segments,
- has_wildcard,
- })
+ Some(ParsedPath { segments })
+}
+
+/// Jackson (and therefore Spark) rejects numbers whose digit count exceeds
+/// 1000 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 (see its `ignore_integer`/`ignore_decimal`), so mirror
+/// Jackson's `StreamReadConstraints` counters with a byte scan before parsing:
+/// the sign and the decimal point do not count, integers are limited by their
+/// digit count, and floats by the sum of their integer-part (a lone leading
+/// zero counts as zero digits), fraction and exponent digit counts.
+/// Find the end of a string body starting at `i` (just past the opening
+/// quote). Short bodies are scanned inline; long bodies use memchr2, the same
+/// approach as serde_json's `ignore_str`. Returns the index just past the
+/// closing quote, or None for an unterminated string (the parser rejects the
+/// document anyway).
+#[inline]
+fn skip_string_body(bytes: &[u8], mut i: usize) -> Option<usize> {
+ const SHORT_STRING: usize = 32;
+ if bytes.len() - i <= SHORT_STRING {
+ while i < bytes.len() {
+ match bytes[i] {
+ b'"' => return Some(i + 1),
+ b'\\' => i += 2,
+ _ => i += 1,
+ }
+ }
+ return None;
+ }
+ loop {
+ match memchr::memchr2(b'"', b'\\', &bytes[i..]) {
+ Some(off) if bytes[i + off] == b'"' => return Some(i + off + 1),
+ Some(off) => i += off + 2, // escaped byte
+ None => return None,
+ }
+ }
+}
+
+fn has_oversized_number(json: &str) -> bool {
+ const MAX_NUMBER_DIGITS: usize = 1000;
Review Comment:
For `{"a":1,"b":<1001 nines>}` with `$.a`, Spark 3.4.3 and the base return `1`, while this head returns NULL. I verified both evaluators. [Spark 3.4.3 uses Jackson 2.14.2](https://github.com/apache/spark/blob/v3.4.3/pom.xml), which has no such limit. Please preserve the version-specific behavior and add a Spark 3.4 regression while retaining the 4.1 validation.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on code in PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#discussion_r4055793241
##########
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:
Fixed in 38429ffb — the scan now counts digits exactly the way jackson-core 2.21.2 does, per `ParserBase.resetInt`/`resetFloat`: the sign and decimal point do not count; integers are limited by their digit count; floats by the sum of the integer-part, fraction and exponent digit counts.
Probing the pinned jackson-core directly (Spark 4.1.3's `fasterxml.jackson.version` is 2.21.2) pinned down one more corner: a lone leading-zero integer part counts as zero digits, except when both a fraction and an exponent are present, where it counts as one — `0.5e` followed by 999 exponent digits is rejected with "Number value length (1001) exceeds the maximum allowed (1000)", while `0.` + 1000 fraction digits and `0e` + 1000 exponent digits are accepted. The scan mirrors all of this and it is covered in `get_json_object.sql` (signed, fractional and exponent boundary columns) and in unit tests.
Validation: a 69-case battery of these boundary shapes (plus string-content and escaped-quote controls) run through Spark 4.1.3's evaluator matches on every case that is not already a pre-existing `$`-materialization divergence.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#discussion_r4100699856
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -372,25 +644,107 @@ impl<'de> Visitor<'de> for SegmentVisitor<'_> {
}
fn visit_seq<A: SeqAccess<'de>>(self, mut seq: A) -> Result<Self::Value, A::Error> {
- let PathSegment::Index(idx) = &self.segments[0] else {
- IgnoredAny.visit_seq(seq)?;
- return Ok(None);
- };
-
- for _ in 0..*idx {
- if seq.next_element::<IgnoredAny>()?.is_none() {
- return Ok(None);
+ match &self.segments[0] {
+ PathSegment::Index(idx) => {
+ // Spark switches to Quoted style for the "one or more results"
+ // case: an index immediately followed by a subscript wildcard.
+ let child_style = match self.segments.get(1) {
+ Some(PathSegment::SubscriptWildcard) | Some(PathSegment::DoubleWildcard) => {
+ Style::Quoted
+ }
+ _ => self.style,
+ };
+ for _ in 0..*idx {
+ if seq.next_element::<IgnoredAny>()?.is_none() {
+ return Ok(PathResult::default());
+ }
+ }
+ let found = seq
+ .next_element_seed(PathSeed {
+ segments: &self.segments[1..],
+ style: child_style,
+ reject_direct_null: false,
+ })?
+ .unwrap_or_default();
+ // The remaining elements are still visited, so that a malformed element
+ // after the match yields no match, as a full parse would.
+ IgnoredAny.visit_seq(seq)?;
+ Ok(found)
+ }
+ PathSegment::DoubleWildcard => {
+ // Spark consumes both wildcards of `[*][*]` at once: the
+ // remaining path applies to the outer elements in flatten
+ // style, and the collected writes always form a single array,
+ // even when there is only one element or none matched.
+ let mut writes = SmallVec::new();
+ let mut matched = false;
+ while let Some(mut result) = seq.next_element_seed(PathSeed {
+ segments: &self.segments[1..],
+ style: Style::Flatten,
+ reject_direct_null: false,
+ })? {
+ matched |= result.matched;
+ writes.append(&mut result.writes);
+ }
+ Ok(PathResult::wrap(writes, matched))
+ }
+ PathSegment::SubscriptWildcard => match self.style {
+ // Quoted style: the array wrapper is always kept, even for a
+ // single match.
+ Style::Quoted => {
+ let mut writes = SmallVec::new();
+ let mut matched = false;
+ while let Some(mut result) = seq.next_element_seed(PathSeed {
+ segments: &self.segments[1..],
+ style: Style::Quoted,
+ reject_direct_null: false,
+ })? {
+ matched |= result.matched;
+ writes.append(&mut result.writes);
+ }
+ Ok(PathResult::wrap(writes, matched))
+ }
+ // Raw or Flatten style: Spark buffers the element writes into
+ // a temporary array and only emits it when more than one
+ // element wrote; a lone writer's brackets are stripped, and
+ // nothing at all is written when no element matched.
+ Style::Raw | Style::Flatten => {
+ let child_style = if self.style == Style::Raw {
+ Style::Quoted
+ } else {
+ Style::Flatten
+ };
+ let mut writers = 0;
+ let mut writes = SmallVec::new();
+ while let Some(mut result) = seq.next_element_seed(PathSeed {
Review Comment:
[P2] Preserve Spark's nesting limit when streaming wildcard paths
For `$[*].a`, construct the valid JSON as `'[{"a":1,"skip":' + '[' * 999 + '0' + ']' * 999 + '}]'`. Its total nesting depth is 1,001. Spark 3.5.9, 4.0.4, 4.1.3, and the PR base return SQL NULL, while this head returns `1`. I reproduced the head result through the compiled scalar and both column entry points. Reducing the inner arrays to 998 makes Spark and head return `1`; either field order reproduces the boundary.
The new wildcard traversal skips unrelated subtrees through `IgnoredAny`, which does not enforce Jackson's depth constraint. This extends a pre-existing nonwildcard limitation to wildcard paths that the base rejected.
Could we enforce the applicable Spark version's nesting limit while skipping subtrees and add both sides of this boundary? Spark 3.4.3 accepts these documents, so the check needs to be version-aware. Restoring serde's older 128-level limit would be too restrictive.
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -246,68 +306,269 @@ fn parse_json_path(path: &str) -> Option<ParsedPath> {
}
}
- Some(ParsedPath {
- segments,
- has_wildcard,
- })
+ Some(ParsedPath { segments })
+}
+
+/// Find the end of a string body starting at `i` (just past the opening
+/// quote). Short bodies are scanned inline; long bodies use memchr2, the same
+/// approach as serde_json's `ignore_str`. Returns the index just past the
+/// closing quote, or None for an unterminated string (the parser rejects the
+/// document anyway).
+#[inline]
+fn skip_string_body(bytes: &[u8], mut i: usize) -> Option<usize> {
+ const SHORT_STRING: usize = 32;
+ if bytes.len().checked_sub(i)? <= SHORT_STRING {
+ while i < bytes.len() {
+ match bytes[i] {
+ b'"' => return Some(i + 1),
+ b'\\' => i = i.checked_add(2)?,
+ _ => i += 1,
+ }
+ }
+ return None;
+ }
+ loop {
+ match memchr::memchr2(b'"', b'\\', bytes.get(i..)?) {
+ Some(off) if bytes[i + off] == b'"' => return Some(i + off + 1),
+ Some(off) => i = i.checked_add(off)?.checked_add(2)?, // escaped byte
+ None => return None,
+ }
+ }
+}
+
+/// Spark 3.5+ bundles Jackson versions that reject numbers beyond the default
+/// 1000-digit limit, including values this evaluation skips. serde_json's
+/// `IgnoredAny` enforces no such limit, so inspect number tokens before parsing.
+/// Spark 3.4's Jackson has no default limit and bypasses this scan.
+fn has_oversized_number(json: &str) -> bool {
+ const MAX_NUMBER_DIGITS: usize = 1000;
+ const JACKSON_READER_BUFFER_UNITS: usize = 4000;
+ let bytes = json.as_bytes();
+ let mut i = 0;
+ // Only near-limit floats need the Reader's UTF-16 position. Keep the
+ // prefix count between candidates so many long numbers stay linear.
+ let mut counted_through = 0;
+ let mut utf16_units = 0;
+ while i < bytes.len() {
+ match bytes[i] {
+ // Skip string bodies: Jackson applies no numeric constraint to
+ // string content.
+ b'"' => match skip_string_body(bytes, i + 1) {
+ Some(end) => i = end,
+ None => return false,
+ },
+ b'-' | b'0'..=b'9' => {
+ let mut j = i + usize::from(bytes[i] == b'-');
+ let int_start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ let int_len = j - int_start;
+ let mut fract_len = 0;
+ if j < bytes.len() && bytes[j] == b'.' {
+ j += 1;
+ let start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ fract_len = j - start;
+ }
+ let mut exp_len = 0;
+ if j < bytes.len() && (bytes[j] | 0x20) == b'e' {
+ j += 1;
+ if j < bytes.len() && (bytes[j] == b'+' || bytes[j] == b'-') {
+ j += 1;
+ }
+ let start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ exp_len = j - start;
+ }
+ let is_float = fract_len > 0 || exp_len > 0;
+ let digit_count = if is_float {
+ // Spark parses UTF8String via InputStreamReader, then
+ // ReaderBasedJsonParser with a 4000-UTF-16-unit buffer.
+ // Its slow path uses -1 for an absent fraction or exponent,
+ // forgiving one digit when a number reaches a buffer edge
+ // or EOF. Numbers starting with zero always take that path.
+ let starts_with_zero = int_len == 1 && bytes[int_start] == b'0';
+ let reaches_buffer_edge = if int_len + fract_len + exp_len > MAX_NUMBER_DIGITS {
+ utf16_units += json[counted_through..i].encode_utf16().count();
+ counted_through = i;
+ utf16_units % JACKSON_READER_BUFFER_UNITS + (j - i)
Review Comment:
[P2] Account for recycled Jackson buffers in numeric validation
This assumes every reader buffer has 4,000 UTF-16 units, but Jackson can reuse a larger buffer allocated by an earlier String parser. For `{"a":1,"pad":"<5000 x characters>","n":1.<1000 zeros>}`, `$.a` and `$.n` return `1` and `1.0` on the base and actual Spark with a recycled 6,000-character buffer. This head returns NULL for both, including through the compiled scalar and both column entry points.
I reproduced the larger-buffer behavior on Spark 3.5.9, 4.0.4, and 4.1.3 evaluators. It also occurs in ordinary Spark 4.1.3 SQL: read a 6,000-character JSON Dataset row, build this JSON from a nonconstant input column, then apply `get_json_object`. It reproduces with whole-stage codegen disabled and enabled. [JsonFactory allocates the String parser buffer by input length](https://github.com/FasterXML/jackson-core/blob/jackson-core-2.21.2/src/main/java/com/fasterxml/jackson/core/JsonFactory.java#L1362-L1373), and [BufferRecycler retains larger buffers](https://github.com/FasterXML/jackson-core/blob/jackson-core-2.21.2/src/main/java/com/fasterxml/jackson/core/util/BufferRecycler.java#L190-L207).
Could we revisit this fixed-buffer compatibility model and cover the JSON Dataset pipeline? A fixed modulo cannot recover the JVM recycler state from the JSON text. An explicit compatibility policy for these rare long-number inputs would also be clearer than claiming reader-boundary parity.
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -246,68 +306,269 @@ fn parse_json_path(path: &str) -> Option<ParsedPath> {
}
}
- Some(ParsedPath {
- segments,
- has_wildcard,
- })
+ Some(ParsedPath { segments })
+}
+
+/// Find the end of a string body starting at `i` (just past the opening
+/// quote). Short bodies are scanned inline; long bodies use memchr2, the same
+/// approach as serde_json's `ignore_str`. Returns the index just past the
+/// closing quote, or None for an unterminated string (the parser rejects the
+/// document anyway).
+#[inline]
+fn skip_string_body(bytes: &[u8], mut i: usize) -> Option<usize> {
+ const SHORT_STRING: usize = 32;
+ if bytes.len().checked_sub(i)? <= SHORT_STRING {
+ while i < bytes.len() {
+ match bytes[i] {
+ b'"' => return Some(i + 1),
+ b'\\' => i = i.checked_add(2)?,
+ _ => i += 1,
+ }
+ }
+ return None;
+ }
+ loop {
+ match memchr::memchr2(b'"', b'\\', bytes.get(i..)?) {
+ Some(off) if bytes[i + off] == b'"' => return Some(i + off + 1),
+ Some(off) => i = i.checked_add(off)?.checked_add(2)?, // escaped byte
+ None => return None,
+ }
+ }
+}
+
+/// Spark 3.5+ bundles Jackson versions that reject numbers beyond the default
+/// 1000-digit limit, including values this evaluation skips. serde_json's
+/// `IgnoredAny` enforces no such limit, so inspect number tokens before parsing.
+/// Spark 3.4's Jackson has no default limit and bypasses this scan.
+fn has_oversized_number(json: &str) -> bool {
+ const MAX_NUMBER_DIGITS: usize = 1000;
+ const JACKSON_READER_BUFFER_UNITS: usize = 4000;
+ let bytes = json.as_bytes();
+ let mut i = 0;
+ // Only near-limit floats need the Reader's UTF-16 position. Keep the
+ // prefix count between candidates so many long numbers stay linear.
+ let mut counted_through = 0;
+ let mut utf16_units = 0;
+ while i < bytes.len() {
+ match bytes[i] {
+ // Skip string bodies: Jackson applies no numeric constraint to
+ // string content.
+ b'"' => match skip_string_body(bytes, i + 1) {
+ Some(end) => i = end,
+ None => return false,
+ },
+ b'-' | b'0'..=b'9' => {
+ let mut j = i + usize::from(bytes[i] == b'-');
+ let int_start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ let int_len = j - int_start;
+ let mut fract_len = 0;
+ if j < bytes.len() && bytes[j] == b'.' {
+ j += 1;
+ let start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ fract_len = j - start;
+ }
+ let mut exp_len = 0;
+ if j < bytes.len() && (bytes[j] | 0x20) == b'e' {
+ j += 1;
+ if j < bytes.len() && (bytes[j] == b'+' || bytes[j] == b'-') {
+ j += 1;
+ }
+ let start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ exp_len = j - start;
+ }
+ let is_float = fract_len > 0 || exp_len > 0;
+ let digit_count = if is_float {
+ // Spark parses UTF8String via InputStreamReader, then
+ // ReaderBasedJsonParser with a 4000-UTF-16-unit buffer.
+ // Its slow path uses -1 for an absent fraction or exponent,
+ // forgiving one digit when a number reaches a buffer edge
+ // or EOF. Numbers starting with zero always take that path.
+ let starts_with_zero = int_len == 1 && bytes[int_start] == b'0';
+ let reaches_buffer_edge = if int_len + fract_len + exp_len > MAX_NUMBER_DIGITS {
+ utf16_units += json[counted_through..i].encode_utf16().count();
+ counted_through = i;
+ utf16_units % JACKSON_READER_BUFFER_UNITS + (j - i)
+ >= JACKSON_READER_BUFFER_UNITS
+ } else {
+ false
+ };
+ let slow_path = starts_with_zero || reaches_buffer_edge || j == bytes.len();
+ let absent_component = fract_len == 0 || exp_len == 0;
+ int_len + fract_len + exp_len - usize::from(slow_path && absent_component)
+ } else {
+ int_len
+ };
+ if digit_count > MAX_NUMBER_DIGITS {
+ return true;
+ }
+ i = j;
+ }
+ _ => i += 1,
+ }
+ }
+ false
}
/// Evaluate a parsed JSONPath against a JSON string.
/// Returns the result as a string, or None if no match.
+#[cfg(test)]
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)?);
- }
-
- let value: Value = serde_json::from_str(json_str).ok()?;
+ evaluate_path_with_number_limit(json_str, path, true)
+}
- // Wildcard path: may return multiple results
- let results = evaluate_with_wildcard(&value, &path.segments);
+fn evaluate_path_with_number_limit(
+ json_str: &str,
+ path: &ParsedPath,
+ check_number_length: bool,
+) -> Option<String> {
+ if check_number_length && has_oversized_number(json_str) {
Review Comment:
[P2] Skip numeric validation for inputs too short to exceed the limit
This scans every document before extraction on Spark 3.5+, even when the entire input is at most 1,000 bytes and therefore cannot contain more than 1,000 ASCII numeric digits.
For a representative 198-byte record with path `$.name`, an independent five-round alternating exact-source benchmark measured 338.54 ns on base versus 547.29 ns on head. Adding only `json_str.len() > 1000` before this scan reduced the result to 356.60 ns, with identical outputs. Two separate seven-round runs corroborated the slowdown. Paths were parsed once and inputs/results were black-boxed; these are evaluator timings, not whole-query timings.
Could we add this length guard? Serde still validates the document's syntax. The earlier bytewise 64 KiB string-scan issue is fixed; this is remaining avoidable work on ordinary short records.
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -246,68 +306,269 @@ fn parse_json_path(path: &str) -> Option<ParsedPath> {
}
}
- Some(ParsedPath {
- segments,
- has_wildcard,
- })
+ Some(ParsedPath { segments })
+}
+
+/// Find the end of a string body starting at `i` (just past the opening
+/// quote). Short bodies are scanned inline; long bodies use memchr2, the same
+/// approach as serde_json's `ignore_str`. Returns the index just past the
+/// closing quote, or None for an unterminated string (the parser rejects the
+/// document anyway).
+#[inline]
+fn skip_string_body(bytes: &[u8], mut i: usize) -> Option<usize> {
+ const SHORT_STRING: usize = 32;
+ if bytes.len().checked_sub(i)? <= SHORT_STRING {
+ while i < bytes.len() {
+ match bytes[i] {
+ b'"' => return Some(i + 1),
+ b'\\' => i = i.checked_add(2)?,
+ _ => i += 1,
+ }
+ }
+ return None;
+ }
+ loop {
+ match memchr::memchr2(b'"', b'\\', bytes.get(i..)?) {
+ Some(off) if bytes[i + off] == b'"' => return Some(i + off + 1),
+ Some(off) => i = i.checked_add(off)?.checked_add(2)?, // escaped byte
+ None => return None,
+ }
+ }
+}
+
+/// Spark 3.5+ bundles Jackson versions that reject numbers beyond the default
+/// 1000-digit limit, including values this evaluation skips. serde_json's
+/// `IgnoredAny` enforces no such limit, so inspect number tokens before parsing.
+/// Spark 3.4's Jackson has no default limit and bypasses this scan.
+fn has_oversized_number(json: &str) -> bool {
+ const MAX_NUMBER_DIGITS: usize = 1000;
+ const JACKSON_READER_BUFFER_UNITS: usize = 4000;
+ let bytes = json.as_bytes();
+ let mut i = 0;
+ // Only near-limit floats need the Reader's UTF-16 position. Keep the
+ // prefix count between candidates so many long numbers stay linear.
+ let mut counted_through = 0;
+ let mut utf16_units = 0;
+ while i < bytes.len() {
+ match bytes[i] {
+ // Skip string bodies: Jackson applies no numeric constraint to
+ // string content.
+ b'"' => match skip_string_body(bytes, i + 1) {
+ Some(end) => i = end,
+ None => return false,
+ },
+ b'-' | b'0'..=b'9' => {
+ let mut j = i + usize::from(bytes[i] == b'-');
+ let int_start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ let int_len = j - int_start;
+ let mut fract_len = 0;
+ if j < bytes.len() && bytes[j] == b'.' {
+ j += 1;
+ let start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ fract_len = j - start;
+ }
+ let mut exp_len = 0;
+ if j < bytes.len() && (bytes[j] | 0x20) == b'e' {
+ j += 1;
+ if j < bytes.len() && (bytes[j] == b'+' || bytes[j] == b'-') {
+ j += 1;
+ }
+ let start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ exp_len = j - start;
+ }
+ let is_float = fract_len > 0 || exp_len > 0;
+ let digit_count = if is_float {
+ // Spark parses UTF8String via InputStreamReader, then
+ // ReaderBasedJsonParser with a 4000-UTF-16-unit buffer.
+ // Its slow path uses -1 for an absent fraction or exponent,
+ // forgiving one digit when a number reaches a buffer edge
+ // or EOF. Numbers starting with zero always take that path.
+ let starts_with_zero = int_len == 1 && bytes[int_start] == b'0';
+ let reaches_buffer_edge = if int_len + fract_len + exp_len > MAX_NUMBER_DIGITS {
+ utf16_units += json[counted_through..i].encode_utf16().count();
+ counted_through = i;
+ utf16_units % JACKSON_READER_BUFFER_UNITS + (j - i)
+ >= JACKSON_READER_BUFFER_UNITS
+ } else {
+ false
+ };
+ let slow_path = starts_with_zero || reaches_buffer_edge || j == bytes.len();
+ let absent_component = fract_len == 0 || exp_len == 0;
+ int_len + fract_len + exp_len - usize::from(slow_path && absent_component)
+ } else {
+ int_len
+ };
+ if digit_count > MAX_NUMBER_DIGITS {
+ return true;
+ }
+ i = j;
+ }
+ _ => i += 1,
+ }
+ }
+ false
}
/// Evaluate a parsed JSONPath against a JSON string.
/// Returns the result as a string, or None if no match.
+#[cfg(test)]
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)?);
- }
-
- let value: Value = serde_json::from_str(json_str).ok()?;
+ evaluate_path_with_number_limit(json_str, path, true)
+}
- // Wildcard path: may return multiple results
- let results = evaluate_with_wildcard(&value, &path.segments);
+fn evaluate_path_with_number_limit(
+ json_str: &str,
+ path: &ParsedPath,
+ check_number_length: bool,
+) -> Option<String> {
+ if check_number_length && has_oversized_number(json_str) {
+ return None;
+ }
- match results.len() {
- 0 => None,
- 1 => {
- // Single wildcard match: Spark preserves JSON serialization format
- // (strings keep their quotes, numbers don't)
- if results[0].is_null() {
- None
- } else {
- serde_json::to_string(results[0]).ok()
- }
- }
- // Multiple results: wrap in JSON array. A slice of `&Value` serializes
- // as a JSON array, so no clone into an owned `Value::Array` is needed.
- _ => serde_json::to_string(&results).ok(),
+ let result = extract_path(json_str, &path.segments)?;
+ if !result.matched {
+ return None;
}
+ // The top level is not an array context. Jackson's generator separates
+ // consecutive root-level writes with a single space, so join with one.
+ Some(PathResult::join(result.writes, " "))
}
-/// Evaluation for paths without wildcards.
-///
/// Descends into the document while it is being parsed, so only the matched
-/// subtree is materialized as a `Value`; everything else is skipped by the
-/// parser without allocating. The whole document is still consumed, so
-/// malformed JSON anywhere in the input yields no match, as a full parse would.
-fn extract_no_wildcard(json_str: &str, segments: &[PathSegment]) -> Option<Value> {
+/// subtrees are materialized as `Value`s; everything else is skipped by the
+/// parser without allocating. The whole document is still consumed, so malformed
+/// JSON anywhere in the input yields no match, as a full parse would.
+fn extract_path(json_str: &str, segments: &[PathSegment]) -> Option<PathResult> {
let mut de = serde_json::Deserializer::from_str(json_str);
- let found = PathSeed { segments }.deserialize(&mut de).ok()?;
+ let found = PathSeed {
+ segments,
+ style: Style::Raw,
+ reject_direct_null: false,
+ }
+ .deserialize(&mut de)
+ .ok()?;
de.end().ok()?;
- found
+ Some(found)
}
/// Deserializes the value at `segments`, discarding everything else.
struct PathSeed<'a> {
segments: &'a [PathSegment],
+ /// The output style in effect, mirroring the `style` parameter Spark
+ /// threads through `evaluatePath`.
+ style: Style,
+ /// A JSON null directly below a named field is not a match in Spark. Nulls
+ /// reached through array traversal are matches and serialize as `null`.
+ reject_direct_null: bool,
+}
+
+/// The outcome of applying (part of) a path, modeled on Spark's generator
+/// protocol: `writes` holds one rendered fragment per generator write and
+/// `matched` is Spark's dirty flag.
+///
+/// The two can diverge: the wildcard arms that write directly to the generator
+/// emit their array wrapper even when nothing inside matched, so an unmatched
+/// result can still carry writes. Spark's generator keeps those bytes — a later
+/// occurrence of a duplicated field can build on them — so they are preserved
+/// here rather than discarded.
+#[derive(Default)]
+struct PathResult {
+ // A simple field or index lookup produces one write; keep it inline while
+ // descending through nested objects and arrays.
+ writes: SmallVec<[String; 1]>,
+ matched: bool,
+}
+
+impl PathResult {
+ fn join(mut writes: SmallVec<[String; 1]>, separator: &str) -> String {
+ if writes.len() == 1 {
+ writes.pop().unwrap()
+ } else {
+ writes.join(separator)
+ }
+ }
+
+ /// A single verbatim write of a matched value, honoring the output style:
+ /// a string in Raw style is written unquoted (Spark's scalar-unwrap arm),
+ /// everything else keeps JSON serialization.
+ fn write(value: Value, style: Style) -> Self {
+ match value {
+ Value::String(s) if style == Style::Raw => Self {
+ writes: smallvec![s],
+ matched: true,
+ },
+ value => Self {
+ writes: smallvec![value.to_string()],
Review Comment:
[P2] Avoid per-leaf temporary serialization for dense wildcard results
Each selected wildcard leaf is serialized into its own String here, then accumulated and copied into the wrapper at line 528. Repeated alternating exact-source benchmarks against the PR base show 1,000-number wildcard extraction at about 1.8-2.1x base runtime, and 1,000-string extraction at 1.65-1.69x. An independent five-round check measured 54.02 us to 99.57 us for numbers and 94.23 us to 157.10 us for strings. The regression persists with numeric validation disabled, so it is separate from the pre-scan issue.
The singleton ownership fix works, and numeric-wildcard allocation counts have improved. This finding concerns the remaining serialization/copy CPU cost. Sparse wildcard objects improve through selective traversal, so keeping that behavior matters.
Could we add a common terminal-wildcard path that writes directly into an output buffer while preserving the existing per-level styles, match counts, and duplicate-field side effects? These measurements use cached parsed paths and black-boxed inputs/results and establish evaluator regressions, not whole-query timings.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on code in PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#discussion_r4055797041
##########
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:
Fixed in 38429ffb. String bodies are no longer walked byte by byte: short bodies are scanned inline and longer ones use memchr2 — the same approach as serde_json's own `ignore_str`, which is the floor a separate validation pass can reach. Integrating the check into the parse itself is not possible through serde's API: `Read` is a sealed trait and visitors see numbers only after the token has been parsed (the length is gone by `visit_f64`), which is why the skipped-value path imposes no limit in the first place.
Measured with the added `large_skipped_string` criterion benchmark (64 KiB unselected string, `$.a`, the same shape as your harness): 9.08 us/doc without the scan versus 9.6 us/doc with it on this machine, down from the ~117 us the byte-wise state machine cost. On the existing realistic-document benches the overhead is about a quarter (339 -> 427 ns/doc for `$.name`); a document only pays for the bytes the scan has to touch, and the scan exits at the first violation.
Your three-round alternating methodology is much better than a single before/after run — glad to adopt it for the numbers above.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on code in PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#discussion_r4133228690
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -246,68 +306,269 @@ fn parse_json_path(path: &str) -> Option<ParsedPath> {
}
}
- Some(ParsedPath {
- segments,
- has_wildcard,
- })
+ Some(ParsedPath { segments })
+}
+
+/// Find the end of a string body starting at `i` (just past the opening
+/// quote). Short bodies are scanned inline; long bodies use memchr2, the same
+/// approach as serde_json's `ignore_str`. Returns the index just past the
+/// closing quote, or None for an unterminated string (the parser rejects the
+/// document anyway).
+#[inline]
+fn skip_string_body(bytes: &[u8], mut i: usize) -> Option<usize> {
+ const SHORT_STRING: usize = 32;
+ if bytes.len().checked_sub(i)? <= SHORT_STRING {
+ while i < bytes.len() {
+ match bytes[i] {
+ b'"' => return Some(i + 1),
+ b'\\' => i = i.checked_add(2)?,
+ _ => i += 1,
+ }
+ }
+ return None;
+ }
+ loop {
+ match memchr::memchr2(b'"', b'\\', bytes.get(i..)?) {
+ Some(off) if bytes[i + off] == b'"' => return Some(i + off + 1),
+ Some(off) => i = i.checked_add(off)?.checked_add(2)?, // escaped byte
+ None => return None,
+ }
+ }
+}
+
+/// Spark 3.5+ bundles Jackson versions that reject numbers beyond the default
+/// 1000-digit limit, including values this evaluation skips. serde_json's
+/// `IgnoredAny` enforces no such limit, so inspect number tokens before parsing.
+/// Spark 3.4's Jackson has no default limit and bypasses this scan.
+fn has_oversized_number(json: &str) -> bool {
+ const MAX_NUMBER_DIGITS: usize = 1000;
+ const JACKSON_READER_BUFFER_UNITS: usize = 4000;
+ let bytes = json.as_bytes();
+ let mut i = 0;
+ // Only near-limit floats need the Reader's UTF-16 position. Keep the
+ // prefix count between candidates so many long numbers stay linear.
+ let mut counted_through = 0;
+ let mut utf16_units = 0;
+ while i < bytes.len() {
+ match bytes[i] {
+ // Skip string bodies: Jackson applies no numeric constraint to
+ // string content.
+ b'"' => match skip_string_body(bytes, i + 1) {
+ Some(end) => i = end,
+ None => return false,
+ },
+ b'-' | b'0'..=b'9' => {
+ let mut j = i + usize::from(bytes[i] == b'-');
+ let int_start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ let int_len = j - int_start;
+ let mut fract_len = 0;
+ if j < bytes.len() && bytes[j] == b'.' {
+ j += 1;
+ let start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ fract_len = j - start;
+ }
+ let mut exp_len = 0;
+ if j < bytes.len() && (bytes[j] | 0x20) == b'e' {
+ j += 1;
+ if j < bytes.len() && (bytes[j] == b'+' || bytes[j] == b'-') {
+ j += 1;
+ }
+ let start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ exp_len = j - start;
+ }
+ let is_float = fract_len > 0 || exp_len > 0;
+ let digit_count = if is_float {
+ // Spark parses UTF8String via InputStreamReader, then
+ // ReaderBasedJsonParser with a 4000-UTF-16-unit buffer.
+ // Its slow path uses -1 for an absent fraction or exponent,
+ // forgiving one digit when a number reaches a buffer edge
+ // or EOF. Numbers starting with zero always take that path.
+ let starts_with_zero = int_len == 1 && bytes[int_start] == b'0';
+ let reaches_buffer_edge = if int_len + fract_len + exp_len > MAX_NUMBER_DIGITS {
+ utf16_units += json[counted_through..i].encode_utf16().count();
+ counted_through = i;
+ utf16_units % JACKSON_READER_BUFFER_UNITS + (j - i)
+ >= JACKSON_READER_BUFFER_UNITS
+ } else {
+ false
+ };
+ let slow_path = starts_with_zero || reaches_buffer_edge || j == bytes.len();
+ let absent_component = fract_len == 0 || exp_len == 0;
+ int_len + fract_len + exp_len - usize::from(slow_path && absent_component)
+ } else {
+ int_len
+ };
+ if digit_count > MAX_NUMBER_DIGITS {
+ return true;
+ }
+ i = j;
+ }
+ _ => i += 1,
+ }
+ }
+ false
}
/// Evaluate a parsed JSONPath against a JSON string.
/// Returns the result as a string, or None if no match.
+#[cfg(test)]
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)?);
- }
-
- let value: Value = serde_json::from_str(json_str).ok()?;
+ evaluate_path_with_number_limit(json_str, path, true)
+}
- // Wildcard path: may return multiple results
- let results = evaluate_with_wildcard(&value, &path.segments);
+fn evaluate_path_with_number_limit(
+ json_str: &str,
+ path: &ParsedPath,
+ check_number_length: bool,
+) -> Option<String> {
+ if check_number_length && has_oversized_number(json_str) {
Review Comment:
done, Skipped the numeric-validation pass for JSON inputs of at most 1,000 bytes; the short-record benchmark is back near baseline.
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -246,68 +306,269 @@ fn parse_json_path(path: &str) -> Option<ParsedPath> {
}
}
- Some(ParsedPath {
- segments,
- has_wildcard,
- })
+ Some(ParsedPath { segments })
+}
+
+/// Find the end of a string body starting at `i` (just past the opening
+/// quote). Short bodies are scanned inline; long bodies use memchr2, the same
+/// approach as serde_json's `ignore_str`. Returns the index just past the
+/// closing quote, or None for an unterminated string (the parser rejects the
+/// document anyway).
+#[inline]
+fn skip_string_body(bytes: &[u8], mut i: usize) -> Option<usize> {
+ const SHORT_STRING: usize = 32;
+ if bytes.len().checked_sub(i)? <= SHORT_STRING {
+ while i < bytes.len() {
+ match bytes[i] {
+ b'"' => return Some(i + 1),
+ b'\\' => i = i.checked_add(2)?,
+ _ => i += 1,
+ }
+ }
+ return None;
+ }
+ loop {
+ match memchr::memchr2(b'"', b'\\', bytes.get(i..)?) {
+ Some(off) if bytes[i + off] == b'"' => return Some(i + off + 1),
+ Some(off) => i = i.checked_add(off)?.checked_add(2)?, // escaped byte
+ None => return None,
+ }
+ }
+}
+
+/// Spark 3.5+ bundles Jackson versions that reject numbers beyond the default
+/// 1000-digit limit, including values this evaluation skips. serde_json's
+/// `IgnoredAny` enforces no such limit, so inspect number tokens before parsing.
+/// Spark 3.4's Jackson has no default limit and bypasses this scan.
+fn has_oversized_number(json: &str) -> bool {
+ const MAX_NUMBER_DIGITS: usize = 1000;
+ const JACKSON_READER_BUFFER_UNITS: usize = 4000;
+ let bytes = json.as_bytes();
+ let mut i = 0;
+ // Only near-limit floats need the Reader's UTF-16 position. Keep the
+ // prefix count between candidates so many long numbers stay linear.
+ let mut counted_through = 0;
+ let mut utf16_units = 0;
+ while i < bytes.len() {
+ match bytes[i] {
+ // Skip string bodies: Jackson applies no numeric constraint to
+ // string content.
+ b'"' => match skip_string_body(bytes, i + 1) {
+ Some(end) => i = end,
+ None => return false,
+ },
+ b'-' | b'0'..=b'9' => {
+ let mut j = i + usize::from(bytes[i] == b'-');
+ let int_start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ let int_len = j - int_start;
+ let mut fract_len = 0;
+ if j < bytes.len() && bytes[j] == b'.' {
+ j += 1;
+ let start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ fract_len = j - start;
+ }
+ let mut exp_len = 0;
+ if j < bytes.len() && (bytes[j] | 0x20) == b'e' {
+ j += 1;
+ if j < bytes.len() && (bytes[j] == b'+' || bytes[j] == b'-') {
+ j += 1;
+ }
+ let start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ exp_len = j - start;
+ }
+ let is_float = fract_len > 0 || exp_len > 0;
+ let digit_count = if is_float {
+ // Spark parses UTF8String via InputStreamReader, then
+ // ReaderBasedJsonParser with a 4000-UTF-16-unit buffer.
+ // Its slow path uses -1 for an absent fraction or exponent,
+ // forgiving one digit when a number reaches a buffer edge
+ // or EOF. Numbers starting with zero always take that path.
+ let starts_with_zero = int_len == 1 && bytes[int_start] == b'0';
+ let reaches_buffer_edge = if int_len + fract_len + exp_len > MAX_NUMBER_DIGITS {
+ utf16_units += json[counted_through..i].encode_utf16().count();
+ counted_through = i;
+ utf16_units % JACKSON_READER_BUFFER_UNITS + (j - i)
+ >= JACKSON_READER_BUFFER_UNITS
+ } else {
+ false
+ };
+ let slow_path = starts_with_zero || reaches_buffer_edge || j == bytes.len();
+ let absent_component = fract_len == 0 || exp_len == 0;
+ int_len + fract_len + exp_len - usize::from(slow_path && absent_component)
+ } else {
+ int_len
+ };
+ if digit_count > MAX_NUMBER_DIGITS {
+ return true;
+ }
+ i = j;
+ }
+ _ => i += 1,
+ }
+ }
+ false
}
/// Evaluate a parsed JSONPath against a JSON string.
/// Returns the result as a string, or None if no match.
+#[cfg(test)]
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)?);
- }
-
- let value: Value = serde_json::from_str(json_str).ok()?;
+ evaluate_path_with_number_limit(json_str, path, true)
+}
- // Wildcard path: may return multiple results
- let results = evaluate_with_wildcard(&value, &path.segments);
+fn evaluate_path_with_number_limit(
+ json_str: &str,
+ path: &ParsedPath,
+ check_number_length: bool,
+) -> Option<String> {
+ if check_number_length && has_oversized_number(json_str) {
+ return None;
+ }
- match results.len() {
- 0 => None,
- 1 => {
- // Single wildcard match: Spark preserves JSON serialization format
- // (strings keep their quotes, numbers don't)
- if results[0].is_null() {
- None
- } else {
- serde_json::to_string(results[0]).ok()
- }
- }
- // Multiple results: wrap in JSON array. A slice of `&Value` serializes
- // as a JSON array, so no clone into an owned `Value::Array` is needed.
- _ => serde_json::to_string(&results).ok(),
+ let result = extract_path(json_str, &path.segments)?;
+ if !result.matched {
+ return None;
}
+ // The top level is not an array context. Jackson's generator separates
+ // consecutive root-level writes with a single space, so join with one.
+ Some(PathResult::join(result.writes, " "))
}
-/// Evaluation for paths without wildcards.
-///
/// Descends into the document while it is being parsed, so only the matched
-/// subtree is materialized as a `Value`; everything else is skipped by the
-/// parser without allocating. The whole document is still consumed, so
-/// malformed JSON anywhere in the input yields no match, as a full parse would.
-fn extract_no_wildcard(json_str: &str, segments: &[PathSegment]) -> Option<Value> {
+/// subtrees are materialized as `Value`s; everything else is skipped by the
+/// parser without allocating. The whole document is still consumed, so malformed
+/// JSON anywhere in the input yields no match, as a full parse would.
+fn extract_path(json_str: &str, segments: &[PathSegment]) -> Option<PathResult> {
let mut de = serde_json::Deserializer::from_str(json_str);
- let found = PathSeed { segments }.deserialize(&mut de).ok()?;
+ let found = PathSeed {
+ segments,
+ style: Style::Raw,
+ reject_direct_null: false,
+ }
+ .deserialize(&mut de)
+ .ok()?;
de.end().ok()?;
- found
+ Some(found)
}
/// Deserializes the value at `segments`, discarding everything else.
struct PathSeed<'a> {
segments: &'a [PathSegment],
+ /// The output style in effect, mirroring the `style` parameter Spark
+ /// threads through `evaluatePath`.
+ style: Style,
+ /// A JSON null directly below a named field is not a match in Spark. Nulls
+ /// reached through array traversal are matches and serialize as `null`.
+ reject_direct_null: bool,
+}
+
+/// The outcome of applying (part of) a path, modeled on Spark's generator
+/// protocol: `writes` holds one rendered fragment per generator write and
+/// `matched` is Spark's dirty flag.
+///
+/// The two can diverge: the wildcard arms that write directly to the generator
+/// emit their array wrapper even when nothing inside matched, so an unmatched
+/// result can still carry writes. Spark's generator keeps those bytes — a later
+/// occurrence of a duplicated field can build on them — so they are preserved
+/// here rather than discarded.
+#[derive(Default)]
+struct PathResult {
+ // A simple field or index lookup produces one write; keep it inline while
+ // descending through nested objects and arrays.
+ writes: SmallVec<[String; 1]>,
+ matched: bool,
+}
+
+impl PathResult {
+ fn join(mut writes: SmallVec<[String; 1]>, separator: &str) -> String {
+ if writes.len() == 1 {
+ writes.pop().unwrap()
+ } else {
+ writes.join(separator)
+ }
+ }
+
+ /// A single verbatim write of a matched value, honoring the output style:
+ /// a string in Raw style is written unquoted (Spark's scalar-unwrap arm),
+ /// everything else keeps JSON serialization.
+ fn write(value: Value, style: Style) -> Self {
+ match value {
+ Value::String(s) if style == Style::Raw => Self {
+ writes: smallvec![s],
+ matched: true,
+ },
+ value => Self {
+ writes: smallvec![value.to_string()],
Review Comment:
done, Terminal wildcards now serialize into one shared output buffer while preserving Spark styles and match behavior; added regressions and benchmarks.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on code in PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#discussion_r3974967321
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -320,50 +339,52 @@ struct SegmentVisitor<'a> {
}
impl<'de> Visitor<'de> for SegmentVisitor<'_> {
- type Value = Option<Value>;
+ type Value = PathResult;
fn expecting(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_str("a JSON value")
}
fn visit_bool<E>(self, _: bool) -> Result<Self::Value, E> {
- Ok(None)
+ Ok(PathResult::default())
}
fn visit_i64<E>(self, _: i64) -> Result<Self::Value, E> {
- Ok(None)
+ Ok(PathResult::default())
}
fn visit_u64<E>(self, _: u64) -> Result<Self::Value, E> {
- Ok(None)
+ Ok(PathResult::default())
}
fn visit_f64<E>(self, _: f64) -> Result<Self::Value, E> {
- Ok(None)
+ Ok(PathResult::default())
}
fn visit_str<E>(self, _: &str) -> Result<Self::Value, E> {
- Ok(None)
+ Ok(PathResult::default())
}
fn visit_unit<E>(self) -> Result<Self::Value, E> {
- Ok(None)
+ Ok(PathResult::default())
}
fn visit_map<A: MapAccess<'de>>(self, mut map: A) -> Result<Self::Value, A::Error> {
let PathSegment::Field(name) = &self.segments[0] else {
IgnoredAny.visit_map(map)?;
- return Ok(None);
+ return Ok(PathResult::default());
};
- let mut found = None;
- // Every entry is visited so that a duplicated key resolves to its last
- // occurrence, as it would in a parsed object.
+ let mut found = PathResult::default();
while let Some(matched) = map.next_key_seed(KeySeed(name))? {
- if matched {
- found = map.next_value_seed(PathSeed {
+ if matched && !found.matched {
Review Comment:
Good catch — confirmed against Spark 4.1.3, and the root cause was deeper than the duplicate-key lock: Spark never treats `[*][*]` as two wildcards. Its parser emits `Subscript :: Wildcard :: Subscript :: Wildcard`, and `evaluatePath` consumes both at once (the "non-structure preserving double wildcard" case in `JsonExpressionEvalUtils`), applying the remaining path to the outer array's *elements themselves* in flatten style. So for `{"a":[[{"b":1}]],"a":null}` with `$.a[*][*].b`, the first `a`'s outer element `[{"b":1}]` is an array and cannot match `.b` — nothing is written, `dirty` stays false — and the second `a` is null, hence SQL NULL.
Fixed in the latest commit: `[*][*]` now parses to a dedicated `DoubleWildcard` segment. The remaining path is applied to the outer elements with Spark's flatten style (array leaves are spliced recursively; an array that flattens to nothing writes no leaf nodes, so it is not a match), and the collected matches are always wrapped in a single array, even when there is only one — matching Spark's generator, which unconditionally wraps this case.
Regression coverage:
- `test_duplicate_key_double_wildcard_match_decision` covers this exact input (`-> NULL`), the same shape without the duplicate key, and the fall-through to a later occurrence that does match (`{"a":[[{"b":1}]],"a":[{"b":2}]}` -> `[2]`).
- Five more unit tests pin the flatten semantics (one-level flatten mirroring Spark's `$.store.basket[*][*]` suite case, single match staying wrapped, empty-flatten no-match, objects under `[*][*].b`, recursive flatten).
- Added `[*][*]` queries — including this exact query — to `spark/src/test/resources/sql-tests/expressions/string/get_json_object.sql`, so CI validates against Spark itself. `CometSqlFileTestSuite` passes locally on Spark 4.1.3.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#issuecomment-5724067109
The previous CI failure in `Spark SQL Tests (Spark 4.1) / spark-sql-sql_hive-2` was a transient network error during sbt dependency resolution (`java.net.SocketException: Connection reset` while downloading `org.ow2.asm:asm:9.9` from Maven Central), not a test failure — the run never reached compilation or tests.
I pushed an empty commit to retrigger CI, but the new run is awaiting maintainer approval. Could a committer please approve the workflow run (or re-run the failed job)? Thanks!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#issuecomment-5447794989
Thanks for the review. I checked each point against Spark's `GetJsonObjectEvaluator` (`sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/json/JsonExpressionEvalUtils.scala`).
**1. Lock semantics — the code matches Spark; it was the description that was stale**
Spark's object loop (lines 478-488) only skips later fields once `dirty` is set, and `dirty` is set only when `evaluatePath` returns `true`, i.e. something was actually written. A named field whose value is JSON null returns `false` explicitly (lines 554-560: `if (p.nextToken() != JsonToken.VALUE_NULL) ... else false`). Tracing the two cases:
- `{"a":null,"a":2}` / `$.a`: first `a` -> `VALUE_NULL` -> `false` -> `dirty` stays false -> the second `a` is consulted -> Spark returns `2`, not NULL.
- `{"a":{"x":1},"a":{"b":2}}` / `$.a.b`: the first `a`'s object contains no `b` -> the inner object loop returns `false` -> `dirty` stays false -> the second `a` is consulted -> Spark returns `2`.
So guarding on "key was seen" (as the old description described) would actually diverge from Spark; locking on the first *successful* match is the correct semantics. A test for `{"a":null,"a":2}` already exists — `test_duplicate_key_first_successful_match_wins` covers exactly that input (`-> Some("2")`), along with the nested and null-subpath variants. The PR description was written for the first commit and didn't reflect the second one; I've now updated it to describe the successful-match semantics.
**2. Scope of the change**
Fair point — the description now covers the `PathResult` refactor, `reject_direct_null`, and the streaming wildcard rewrite. One correction: `$.a` on `{"a":null}` is unchanged (SQL NULL before and after — previously via `value_into_string(Null) -> None`, now via `reject_direct_null`). What actually changes is a null reached *through array traversal or a wildcard*: `{"a":[null]}` / `$.a[0]` (and single-match `$.a[*]`) now serialize as the text `null` instead of SQL NULL, matching Spark where `copyCurrentStructure` writes `null` and counts as a match. I've added `test_null_reached_through_array_serializes_as_null_text` covering the non-duplicate-key case.
**3. `visit_seq` performance note**
The trailing `IgnoredAny.visit_seq(seq)` after the matched index predates this PR — it was introduced in #4907 (the `-` side of this diff contains the same call and comment), so `$[0]` scanning to the end of the array is existing behavior, not a regression introduced here. Agreed it may be worth benchmarking separately.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on code in PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#discussion_r4133214916
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -246,68 +306,269 @@ fn parse_json_path(path: &str) -> Option<ParsedPath> {
}
}
- Some(ParsedPath {
- segments,
- has_wildcard,
- })
+ Some(ParsedPath { segments })
+}
+
+/// Find the end of a string body starting at `i` (just past the opening
+/// quote). Short bodies are scanned inline; long bodies use memchr2, the same
+/// approach as serde_json's `ignore_str`. Returns the index just past the
+/// closing quote, or None for an unterminated string (the parser rejects the
+/// document anyway).
+#[inline]
+fn skip_string_body(bytes: &[u8], mut i: usize) -> Option<usize> {
+ const SHORT_STRING: usize = 32;
+ if bytes.len().checked_sub(i)? <= SHORT_STRING {
+ while i < bytes.len() {
+ match bytes[i] {
+ b'"' => return Some(i + 1),
+ b'\\' => i = i.checked_add(2)?,
+ _ => i += 1,
+ }
+ }
+ return None;
+ }
+ loop {
+ match memchr::memchr2(b'"', b'\\', bytes.get(i..)?) {
+ Some(off) if bytes[i + off] == b'"' => return Some(i + off + 1),
+ Some(off) => i = i.checked_add(off)?.checked_add(2)?, // escaped byte
+ None => return None,
+ }
+ }
+}
+
+/// Spark 3.5+ bundles Jackson versions that reject numbers beyond the default
+/// 1000-digit limit, including values this evaluation skips. serde_json's
+/// `IgnoredAny` enforces no such limit, so inspect number tokens before parsing.
+/// Spark 3.4's Jackson has no default limit and bypasses this scan.
+fn has_oversized_number(json: &str) -> bool {
+ const MAX_NUMBER_DIGITS: usize = 1000;
+ const JACKSON_READER_BUFFER_UNITS: usize = 4000;
+ let bytes = json.as_bytes();
+ let mut i = 0;
+ // Only near-limit floats need the Reader's UTF-16 position. Keep the
+ // prefix count between candidates so many long numbers stay linear.
+ let mut counted_through = 0;
+ let mut utf16_units = 0;
+ while i < bytes.len() {
+ match bytes[i] {
+ // Skip string bodies: Jackson applies no numeric constraint to
+ // string content.
+ b'"' => match skip_string_body(bytes, i + 1) {
+ Some(end) => i = end,
+ None => return false,
+ },
+ b'-' | b'0'..=b'9' => {
+ let mut j = i + usize::from(bytes[i] == b'-');
+ let int_start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ let int_len = j - int_start;
+ let mut fract_len = 0;
+ if j < bytes.len() && bytes[j] == b'.' {
+ j += 1;
+ let start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ fract_len = j - start;
+ }
+ let mut exp_len = 0;
+ if j < bytes.len() && (bytes[j] | 0x20) == b'e' {
+ j += 1;
+ if j < bytes.len() && (bytes[j] == b'+' || bytes[j] == b'-') {
+ j += 1;
+ }
+ let start = j;
+ while j < bytes.len() && bytes[j].is_ascii_digit() {
+ j += 1;
+ }
+ exp_len = j - start;
+ }
+ let is_float = fract_len > 0 || exp_len > 0;
+ let digit_count = if is_float {
+ // Spark parses UTF8String via InputStreamReader, then
+ // ReaderBasedJsonParser with a 4000-UTF-16-unit buffer.
+ // Its slow path uses -1 for an absent fraction or exponent,
+ // forgiving one digit when a number reaches a buffer edge
+ // or EOF. Numbers starting with zero always take that path.
+ let starts_with_zero = int_len == 1 && bytes[int_start] == b'0';
+ let reaches_buffer_edge = if int_len + fract_len + exp_len > MAX_NUMBER_DIGITS {
+ utf16_units += json[counted_through..i].encode_utf16().count();
+ counted_through = i;
+ utf16_units % JACKSON_READER_BUFFER_UNITS + (j - i)
Review Comment:
done, Removed the fixed-buffer approximation. The opt-in path now uses a deterministic 1,000-digit policy and documents the recycler-dependent difference; exact JVM-history parity is not possible.
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -372,25 +644,107 @@ impl<'de> Visitor<'de> for SegmentVisitor<'_> {
}
fn visit_seq<A: SeqAccess<'de>>(self, mut seq: A) -> Result<Self::Value, A::Error> {
- let PathSegment::Index(idx) = &self.segments[0] else {
- IgnoredAny.visit_seq(seq)?;
- return Ok(None);
- };
-
- for _ in 0..*idx {
- if seq.next_element::<IgnoredAny>()?.is_none() {
- return Ok(None);
+ match &self.segments[0] {
+ PathSegment::Index(idx) => {
+ // Spark switches to Quoted style for the "one or more results"
+ // case: an index immediately followed by a subscript wildcard.
+ let child_style = match self.segments.get(1) {
+ Some(PathSegment::SubscriptWildcard) | Some(PathSegment::DoubleWildcard) => {
+ Style::Quoted
+ }
+ _ => self.style,
+ };
+ for _ in 0..*idx {
+ if seq.next_element::<IgnoredAny>()?.is_none() {
+ return Ok(PathResult::default());
+ }
+ }
+ let found = seq
+ .next_element_seed(PathSeed {
+ segments: &self.segments[1..],
+ style: child_style,
+ reject_direct_null: false,
+ })?
+ .unwrap_or_default();
+ // The remaining elements are still visited, so that a malformed element
+ // after the match yields no match, as a full parse would.
+ IgnoredAny.visit_seq(seq)?;
+ Ok(found)
+ }
+ PathSegment::DoubleWildcard => {
+ // Spark consumes both wildcards of `[*][*]` at once: the
+ // remaining path applies to the outer elements in flatten
+ // style, and the collected writes always form a single array,
+ // even when there is only one element or none matched.
+ let mut writes = SmallVec::new();
+ let mut matched = false;
+ while let Some(mut result) = seq.next_element_seed(PathSeed {
+ segments: &self.segments[1..],
+ style: Style::Flatten,
+ reject_direct_null: false,
+ })? {
+ matched |= result.matched;
+ writes.append(&mut result.writes);
+ }
+ Ok(PathResult::wrap(writes, matched))
+ }
+ PathSegment::SubscriptWildcard => match self.style {
+ // Quoted style: the array wrapper is always kept, even for a
+ // single match.
+ Style::Quoted => {
+ let mut writes = SmallVec::new();
+ let mut matched = false;
+ while let Some(mut result) = seq.next_element_seed(PathSeed {
+ segments: &self.segments[1..],
+ style: Style::Quoted,
+ reject_direct_null: false,
+ })? {
+ matched |= result.matched;
+ writes.append(&mut result.writes);
+ }
+ Ok(PathResult::wrap(writes, matched))
+ }
+ // Raw or Flatten style: Spark buffers the element writes into
+ // a temporary array and only emits it when more than one
+ // element wrote; a lone writer's brackets are stripped, and
+ // nothing at all is written when no element matched.
+ Style::Raw | Style::Flatten => {
+ let child_style = if self.style == Style::Raw {
+ Style::Quoted
+ } else {
+ Style::Flatten
+ };
+ let mut writers = 0;
+ let mut writes = SmallVec::new();
+ while let Some(mut result) = seq.next_element_seed(PathSeed {
Review Comment:
done, Enforced the Spark 3.5+ nesting limit even in skipped wildcard subtrees; added both boundary cases while preserving Spark 3.4 behavior.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao merged PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#issuecomment-5441692140
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled.
Matching Spark's first-occurrence semantics for duplicate keys is the right fix, and keeping the full traversal so that malformed content after the match still rejects the row is a detail that would have been easy to get wrong.
I think there is a case where the code does not do what the description says, though.
**A first match that resolves to nothing falls through to the second occurrence**
The description says:
> The lock is keyed on the *key match*, not on a successful subpath resolution: if the first matching value does not contain the rest of the path, the result is `null` and the second duplicate key is never consulted, matching Spark.
But the code is:
```rust
while let Some(matched) = map.next_key_seed(KeySeed(name))? {
if matched && !found.matched {
let candidate = map.next_value_seed(PathSeed { segments: &self.segments[1..], reject_direct_null: true })?;
if candidate.matched {
found = candidate;
}
} else {
map.next_value::<IgnoredAny>()?;
}
```
`found.matched` is `PathResult::matched`, which means "the path resolved to a value", not "we saw the key". So when the first occurrence's subpath fails, `found.matched` stays `false` and the loop tries the second occurrence. That is the opposite of the description.
Two concrete cases I would expect to differ from Spark:
```sql
SELECT get_json_object('{"a":{"b":1},"a":{"c":2}}', '$.a.b')
```
Spark stops at the first `a`, finds no `b`, returns `NULL`. This code should skip to the second `a`, find no `b` either, and also return `NULL`, so this one happens to agree. But:
```sql
SELECT get_json_object('{"a":null,"a":2}', '$.a')
```
`reject_direct_null: true` makes the first occurrence return an unmatched `PathResult`, so the loop consults the second and returns `2`. Spark stops at the first `a` and returns `NULL`.
Could you check that second case against Spark? If it does differ, the guard needs to be on whether the key was seen rather than on `found.matched`, which is what the description already describes. Either way a test for `{"a":null,"a":2}` would be worth adding, since it is the shape where the two readings diverge.
**The change is bigger than the description says**
The description talks about `visit_map` locking in the first match. The diff also introduces a `PathResult` type replacing `Option<Value>` throughout, adds a `reject_direct_null` flag with its own semantics for null-below-a-named-field, and rewrites `visit_seq` including the wildcard branch. Those are separate behavioral changes and each deserves a line in the description and its own test, especially `reject_direct_null`, which changes what `$.a` returns for `{"a":null}`.
**A performance note on `visit_seq`**
The `Index` branch now calls `IgnoredAny.visit_seq(seq)` after finding its element, so `$[0]` on a large array scans the whole array instead of stopping at the first element. The comment explains why (a malformed element after the match must reject), and that is correct Spark behavior. Worth measuring on a wide array though, since `$[0]` over a 10k-element array goes from O(1) to O(n) and that could be a visible regression for someone.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#issuecomment-5738573783
x
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on code in PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#discussion_r4133211908
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -246,68 +284,236 @@ fn parse_json_path(path: &str) -> Option<ParsedPath> {
}
}
- Some(ParsedPath {
- segments,
- has_wildcard,
- })
+ Some(ParsedPath { segments })
+}
+
+/// Jackson (and therefore Spark) rejects numbers whose digit count exceeds
+/// 1000 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 (see its `ignore_integer`/`ignore_decimal`), so mirror
+/// Jackson's `StreamReadConstraints` counters with a byte scan before parsing:
+/// the sign and the decimal point do not count, integers are limited by their
+/// digit count, and floats by the sum of their integer-part (a lone leading
+/// zero counts as zero digits), fraction and exponent digit counts.
+/// Find the end of a string body starting at `i` (just past the opening
+/// quote). Short bodies are scanned inline; long bodies use memchr2, the same
+/// approach as serde_json's `ignore_str`. Returns the index just past the
+/// closing quote, or None for an unterminated string (the parser rejects the
+/// document anyway).
+#[inline]
+fn skip_string_body(bytes: &[u8], mut i: usize) -> Option<usize> {
+ const SHORT_STRING: usize = 32;
+ if bytes.len() - i <= SHORT_STRING {
+ while i < bytes.len() {
+ match bytes[i] {
+ b'"' => return Some(i + 1),
+ b'\\' => i += 2,
+ _ => i += 1,
+ }
+ }
+ return None;
+ }
+ loop {
+ match memchr::memchr2(b'"', b'\\', &bytes[i..]) {
+ Some(off) if bytes[i + off] == b'"' => return Some(i + off + 1),
+ Some(off) => i += off + 2, // escaped byte
Review Comment:
done, Bounds-checked truncated escapes so malformed JSON returns NULL; added a regression test.
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -246,68 +284,236 @@ fn parse_json_path(path: &str) -> Option<ParsedPath> {
}
}
- Some(ParsedPath {
- segments,
- has_wildcard,
- })
+ Some(ParsedPath { segments })
+}
+
+/// Jackson (and therefore Spark) rejects numbers whose digit count exceeds
+/// 1000 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 (see its `ignore_integer`/`ignore_decimal`), so mirror
+/// Jackson's `StreamReadConstraints` counters with a byte scan before parsing:
+/// the sign and the decimal point do not count, integers are limited by their
+/// digit count, and floats by the sum of their integer-part (a lone leading
+/// zero counts as zero digits), fraction and exponent digit counts.
+/// Find the end of a string body starting at `i` (just past the opening
+/// quote). Short bodies are scanned inline; long bodies use memchr2, the same
+/// approach as serde_json's `ignore_str`. Returns the index just past the
+/// closing quote, or None for an unterminated string (the parser rejects the
+/// document anyway).
+#[inline]
+fn skip_string_body(bytes: &[u8], mut i: usize) -> Option<usize> {
+ const SHORT_STRING: usize = 32;
+ if bytes.len() - i <= SHORT_STRING {
+ while i < bytes.len() {
+ match bytes[i] {
+ b'"' => return Some(i + 1),
+ b'\\' => i += 2,
+ _ => i += 1,
+ }
+ }
+ return None;
+ }
+ loop {
+ match memchr::memchr2(b'"', b'\\', &bytes[i..]) {
+ Some(off) if bytes[i + off] == b'"' => return Some(i + off + 1),
+ Some(off) => i += off + 2, // escaped byte
+ None => return None,
+ }
+ }
+}
+
+fn has_oversized_number(json: &str) -> bool {
+ const MAX_NUMBER_DIGITS: usize = 1000;
Review Comment:
done, Spark 3.4 now uses a separate native function without the later Jackson numeric limit; added version-specific regressions.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "rich7420 (via GitHub)" <gi...@apache.org>.
rich7420 commented on code in PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#discussion_r3969980424
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -320,50 +339,52 @@ struct SegmentVisitor<'a> {
}
impl<'de> Visitor<'de> for SegmentVisitor<'_> {
- type Value = Option<Value>;
+ type Value = PathResult;
fn expecting(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_str("a JSON value")
}
fn visit_bool<E>(self, _: bool) -> Result<Self::Value, E> {
- Ok(None)
+ Ok(PathResult::default())
}
fn visit_i64<E>(self, _: i64) -> Result<Self::Value, E> {
- Ok(None)
+ Ok(PathResult::default())
}
fn visit_u64<E>(self, _: u64) -> Result<Self::Value, E> {
- Ok(None)
+ Ok(PathResult::default())
}
fn visit_f64<E>(self, _: f64) -> Result<Self::Value, E> {
- Ok(None)
+ Ok(PathResult::default())
}
fn visit_str<E>(self, _: &str) -> Result<Self::Value, E> {
- Ok(None)
+ Ok(PathResult::default())
}
fn visit_unit<E>(self) -> Result<Self::Value, E> {
- Ok(None)
+ Ok(PathResult::default())
}
fn visit_map<A: MapAccess<'de>>(self, mut map: A) -> Result<Self::Value, A::Error> {
let PathSegment::Field(name) = &self.segments[0] else {
IgnoredAny.visit_map(map)?;
- return Ok(None);
+ return Ok(PathResult::default());
};
- let mut found = None;
- // Every entry is visited so that a duplicated key resolves to its last
- // occurrence, as it would in a parsed object.
+ let mut found = PathResult::default();
while let Some(matched) = map.next_key_seed(KeySeed(name))? {
- if matched {
- found = map.next_value_seed(PathSeed {
+ if matched && !found.matched {
Review Comment:
This returns `1` for `{"a":[[{"b":1}]],"a":null}` with `$.a[*][*].b`, while Spark 4.1.3 and the pre-change native UDF return SQL NULL. Could you preserve Spark’s match decision here and add a regression test?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#issuecomment-5902094332
Found **one new P2** in the five-agent review of `87790224` against base `222609d8`.
**[P2] Avoid Revalidating Serialized UTF-8**
[get_json_object.rs:542](https://github.com/apache/datafusion-comet/blob/87790224c8649404448c2a119b5b75ef93056785/native/spark-expr/src/string_funcs/get_json_object.rs#L542) and [line 645](https://github.com/apache/datafusion-comet/blob/87790224c8649404448c2a119b5b75ef93056785/native/spark-expr/src/string_funcs/get_json_object.rs#L645) rescan output that the serializer already guarantees is valid UTF-8.
For `$.a[*]` returning a 65,535-byte CJK string, repeated benchmarks measured **56.87 us on base versus 100.52 us on head: 77% slower**. Removing only redundant validation reduced this to **58.45 us**, with identical output, also verified against Spark. These inputs contain no floats.
Could we preserve the serializer’s documented UTF-8 invariant when constructing the final string and add a non-ASCII output benchmark? These are evaluator timings, not whole-query measurements.
**Other Conclusions**
- The previous four findings are addressed, including the explicitly documented numeric-limit compatibility policy.
- No new actionable correctness issue or unnecessary abstraction was verified.
- No new Spark operator fallback. Native execution remains opt-in; default JVM dispatch is unchanged.
- Float-heavy arrays measured **1.8–1.9x slower** with the Java-style formatter. This is a separate measured compatibility tradeoff.
**Validation**
- Passed **1,033 unit tests**, including 53 JSON tests, plus 8 integration/registration tests.
- Spark 4.1/JDK 17 SQL fixture passed both dictionary configurations using the freshly built native library.
- Checked 155,663 traversal cases, 4,399 numeric cases, 240 edge cases, and 96 compiled-entry-point comparisons. Existing incompatibilities remain.
- Full upstream Spark suites/version matrix were not rerun. [Comet CI](https://github.com/apache/datafusion-comet/actions/runs/36542733719), CodeQL, and title checks remain `action_required`.
I haven’t posted this new finding.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#issuecomment-5804035272
**Four P2 findings remain at head `a1fd4c9`. I would hold approval.** These affect the opt-in native implementation; default JVM execution is unaffected.
1. **Malformed JSON can panic instead of returning NULL.** The escape-handling branch advances without ensuring the next slice stays within bounds. This remains in [get_json_object.rs:319](https://github.com/apache/datafusion-comet/blob/a1fd4c955a6ebbe3ac9551ff49a293c26f4cd4fc/native/spark-expr/src/string_funcs/get_json_object.rs#L319). Confirmed by source inspection; already reported in an unresolved thread.
2. **The unconditional numeric limit regresses Spark 3.4.** An object containing `a:1` and an unselected 1,001-digit integer returns `1` for `$.a` on Spark 3.4.3 and the PR base, but NULL on this head. The [limit at line 326](https://github.com/apache/datafusion-comet/blob/a1fd4c955a6ebbe3ac9551ff49a293c26f4cd4fc/native/spark-expr/src/string_funcs/get_json_object.rs#L326) needs version-aware behavior. Fresh evaluator comparisons confirm the existing report.
3. **Numeric counting still disagrees with Spark 4.1 at reader-buffer boundaries.** With 3,000 padding characters before an unselected `1.` followed by 1,000 fractional digits, Spark 4.1.3 and the base return `1`; head returns NULL. Jackson’s fast and refill paths count absent components differently, which the [digit sum](https://github.com/apache/datafusion-comet/blob/a1fd4c955a6ebbe3ac9551ff49a293c26f4cd4fc/native/spark-expr/src/string_funcs/get_json_object.rs#L381) misses. Reproduced through actual Spark-to-Comet execution with constant and column paths.
4. **Result construction adds substantial allocation overhead.** [PathResult construction](https://github.com/apache/datafusion-comet/blob/a1fd4c955a6ebbe3ac9551ff49a293c26f4cd4fc/native/spark-expr/src/string_funcs/get_json_object.rs#L459), parent appends, and final joining increase small-string extraction from **1 to 4 allocations**, and nested extraction from **1 to 6**. Fresh component benchmarks measured **1.60×**, **1.75×**, and **2.03×** runtime for small strings, nested strings, and a 1,000-number wildcard respectively. Move owned singleton results and avoid intermediate serialization buffers while retaining the required output-style state.
Validation: **47 native tests passed**, and the Spark 4.1 SQL fixture passed both dictionary configurations. Numeric comparisons and the 46,988-case semantic comparison were rerun. Full native rebuilds hit disk exhaustion; successful execution checks used the existing library built from this same head. Performance figures are evaluator measurements, not whole-query timings.
Earlier wrapper and bytewise string-scan findings are fixed. [Comet CI](https://github.com/apache/datafusion-comet/actions/runs/35499100063) and [CodeQL](https://github.com/apache/datafusion-comet/actions/runs/35499099564) still require workflow approval.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#issuecomment-5903289028
done, removed redundant UTF-8 validation in both serialized-output paths and added 65,535-byte CJK tests and benchmarks. Spark 3.4–4.2 focused SQL tests and the SQL Core shard passed (0dadc58fa).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#issuecomment-5876429669
This is a light fully automated review since there are so many PRs open.
Selected floats come back in a different notation from Spark. This predates the PR, but it now lives in the new `PathResult::write`. Spark's generator copies a float token through `double` (Jackson's `_copyCurrentFloatValue` calls `writeNumber(p.getDoubleValue())`, which prints with `Double.toString`), while `value.to_string()` at `native/spark-expr/src/string_funcs/get_json_object.rs:510` and the flatten leaves at line 559 use serde_json's shortest form. So `get_json_object('{"a":0.0001}', '$.a')` returns `1.0E-4` in Spark and `0.0001` natively, and `12345678.9` comes back from Spark as `1.23456789E7`. Floats nested inside a returned object or array differ the same way, since `copyCurrentStructure` copies them through the same method. Unlike the 1,000-digit cases, these are ordinary values. Neither this nor the "selected very large numeric values" gap from the description is in `getIncompatibleReasons()` at `spark/src/main/scala/org/apache/comet/serde/strings.scala:699`, which is
what the generated compatibility page is built from. Could the output go through a `serde_json::ser::Formatter` whose `write_f64` uses the existing `write_java_float_string` in `native/spark-expr/src/conversion_funcs/numeric.rs`, with a few of these values added to `get_json_object.sql`? If that is more than this PR should take on, an issue plus a reasons entry and these queries behind `ignore(<issue link>)` would at least record the gap where the next person will look.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: `get_json_object` returns first value for duplicate keys to match Spark [datafusion-comet]
Posted by "u70b3 (via GitHub)" <gi...@apache.org>.
u70b3 commented on code in PR #4971:
URL: https://github.com/apache/datafusion-comet/pull/4971#discussion_r4051935943
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -255,59 +276,97 @@ fn parse_json_path(path: &str) -> Option<ParsedPath> {
/// 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)?);
+ let mut result = extract_path(json_str, &path.segments)?;
Review Comment:
Fixed in 7099e9d2. Since serde's visitor sees an already-parsed number (the token length is gone by `visit_f64`), the limit is enforced with a pre-parse byte scan mirroring Jackson's `StreamReadConstraints.maxNumberLength = 1000`: any number token longer than 1000 characters anywhere in the document returns null, while 1000-digit tokens are accepted — including digits inside string literals being ignored, which Jackson does not constrain.
Both sides of the boundary are covered in `get_json_object.sql` (via `repeat('9', 1000/1001)`) and in unit tests, for wildcard and non-wildcard paths.
One adjacent pre-existing gap, unchanged by this PR and worth recording: a *selected* number with 309–1000 digits parses fine in Jackson (copied verbatim) but overflows serde_json's f64 during `Value` materialization, so Comet returns null there. Fixing that needs raw-token capture in the materialized value; happy to file a follow-up issue if useful.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org