You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/07/13 16:03:59 UTC
[PR] perf: optimize spark_get_json_object in datafusion-comet-spark-expr [datafusion-comet]
andygrove opened a new pull request, #4907:
URL: https://github.com/apache/datafusion-comet/pull/4907
> This PR was created by an LLM as a draft PR. I will mark it as ready for review after human review.
## Which issue does this PR close?
N/A — autonomous exploratory PR.
## Rationale for this change
Replaced the full serde_json::Value materialization of each row with a streaming DeserializeSeed path walk that skips non-matching subtrees via IgnoredAny and allocates only the matched subtree, eliminating per-key and per-value heap allocations for the whole document.
## What changes are included in this PR?
Replaced the full serde_json::Value materialization of each row with a streaming DeserializeSeed path walk that skips non-matching subtrees via IgnoredAny and allocates only the matched subtree, eliminating per-key and per-value heap allocations for the whole document.
## How are these changes tested?
Correctness: unit tests + seeded differential fuzz (bit-identical Arrow output vs `main`).
Benchmark (criterion):
- nested_field: 75.342% faster (base 7961939ns -> cand 1963265ns)
- top_level_field: 76.842% faster (base 7968457ns -> cand 1845346ns)
- array_index: 75.091% faster (base 7562066ns -> cand 1883639ns)
Full criterion output:
```text
get_json_object/top_level_field
time: [1.8425 ms 1.8451 ms 1.8481 ms]
change: [−76.936% −76.842% −76.753%] (p = 0.00 < 0.05)
Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
3 (3.00%) low mild
3 (3.00%) high mild
4 (4.00%) high severe
get_json_object/nested_field
time: [1.9589 ms 1.9639 ms 1.9682 ms]
change: [−75.504% −75.342% −75.196%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
5 (5.00%) low mild
1 (1.00%) high mild
get_json_object/array_index
time: [1.8813 ms 1.8854 ms 1.8907 ms]
change: [−75.236% −75.091% −74.962%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) high mild
3 (3.00%) high severe
```
--
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] perf: optimize `spark_get_json_object` (4x faster) [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #4907:
URL: https://github.com/apache/datafusion-comet/pull/4907#discussion_r3591172953
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -268,38 +269,151 @@ fn evaluate_path(json_str: &str, path: &ParsedPath) -> Option<String> {
1 => {
// Single wildcard match: Spark preserves JSON serialization format
// (strings keep their quotes, numbers don't)
- let s = serde_json::to_string(results[0]).ok()?;
- if s == "null" {
+ if results[0].is_null() {
None
} else {
- Some(s)
+ serde_json::to_string(results[0]).ok()
}
}
- _ => {
- // Multiple results: wrap in JSON array
- let arr = Value::Array(results.into_iter().cloned().collect());
- Some(arr.to_string())
+ // 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(),
+ }
+}
+
+/// 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> {
+ let mut de = serde_json::Deserializer::from_str(json_str);
+ let found = PathSeed { segments }.deserialize(&mut de).ok()?;
+ de.end().ok()?;
+ found
+}
+
+/// Deserializes the value at `segments`, discarding everything else.
+struct PathSeed<'a> {
+ segments: &'a [PathSegment],
+}
+
+impl<'de> DeserializeSeed<'de> for PathSeed<'_> {
+ type Value = Option<Value>;
+
+ fn deserialize<D: Deserializer<'de>>(self, deserializer: D) -> Result<Self::Value, D::Error> {
+ if self.segments.is_empty() {
+ return Value::deserialize(deserializer).map(Some);
}
+ deserializer.deserialize_any(SegmentVisitor {
+ segments: self.segments,
+ })
}
}
-/// Fast-path evaluation for paths without wildcards.
-/// Returns a reference to the matched value, or None if no match.
-fn evaluate_no_wildcard<'a>(value: &'a Value, segments: &[PathSegment]) -> Option<&'a Value> {
- if segments.is_empty() {
- return Some(value);
+/// Applies `segments[0]` to the value being visited. A value whose shape does
+/// not match the segment (an index into an object, say) is skipped and reported
+/// as no match rather than as an error, matching a lookup on a parsed document.
+struct SegmentVisitor<'a> {
+ segments: &'a [PathSegment],
+}
+
+impl<'de> Visitor<'de> for SegmentVisitor<'_> {
+ type Value = Option<Value>;
+
+ fn expecting(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ f.write_str("a JSON value")
}
- match &segments[0] {
- PathSegment::Field(name) => {
- let child = value.as_object()?.get(name)?;
- evaluate_no_wildcard(child, &segments[1..])
+ fn visit_bool<E>(self, _: bool) -> Result<Self::Value, E> {
+ Ok(None)
+ }
+
+ fn visit_i64<E>(self, _: i64) -> Result<Self::Value, E> {
+ Ok(None)
+ }
+
+ fn visit_u64<E>(self, _: u64) -> Result<Self::Value, E> {
+ Ok(None)
+ }
+
+ fn visit_f64<E>(self, _: f64) -> Result<Self::Value, E> {
+ Ok(None)
+ }
+
+ fn visit_str<E>(self, _: &str) -> Result<Self::Value, E> {
+ Ok(None)
+ }
+
+ fn visit_unit<E>(self) -> Result<Self::Value, E> {
+ Ok(None)
+ }
+
+ 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);
+ };
+
+ 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.
Review Comment:
I filed https://github.com/apache/datafusion-comet/issues/4947
--
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] perf: optimize `spark_get_json_object` (4x faster) [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4907:
URL: https://github.com/apache/datafusion-comet/pull/4907#issuecomment-4985889631
Thanks @mbutrovich . I added the tests and filed https://github.com/apache/datafusion-comet/issues/4947
--
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] perf: optimize `spark_get_json_object` (4x faster) [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #4907:
URL: https://github.com/apache/datafusion-comet/pull/4907
--
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] perf: optimize `spark_get_json_object` (4x faster) [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #4907:
URL: https://github.com/apache/datafusion-comet/pull/4907#discussion_r3590978102
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -268,38 +269,151 @@ fn evaluate_path(json_str: &str, path: &ParsedPath) -> Option<String> {
1 => {
// Single wildcard match: Spark preserves JSON serialization format
// (strings keep their quotes, numbers don't)
- let s = serde_json::to_string(results[0]).ok()?;
- if s == "null" {
+ if results[0].is_null() {
None
} else {
- Some(s)
+ serde_json::to_string(results[0]).ok()
}
}
- _ => {
- // Multiple results: wrap in JSON array
- let arr = Value::Array(results.into_iter().cloned().collect());
- Some(arr.to_string())
+ // 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(),
+ }
+}
+
+/// 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> {
+ let mut de = serde_json::Deserializer::from_str(json_str);
+ let found = PathSeed { segments }.deserialize(&mut de).ok()?;
+ de.end().ok()?;
+ found
+}
Review Comment:
`native/spark-expr/src/string_funcs/get_json_object.rs:290-295` (`extract_no_wildcard`) relies on three things staying true to preserve parity: `de.end()` (line 293) rejecting trailing content, `visit_map` draining every entry after a match (lines 362-370), and `visit_seq` calling `IgnoredAny.visit_seq(seq)` after a match (line 392). These are the load-bearing lines of the whole PR, since they are what makes "streaming stop-early" still behave like "full parse". None of them is covered by a test. If a future refactor drops the drain or the `de.end()`, every existing test still passes because they all use well-formed single-value documents.
Add tests that only pass if the whole document is still validated after the match:
```rust
#[test]
fn test_match_then_trailing_garbage_is_null() {
let path = parse_json_path("$.a").unwrap();
assert_eq!(evaluate_path(r#"{"a":1} garbage"#, &path), None);
}
#[test]
fn test_match_then_malformed_sibling_is_null() {
let path = parse_json_path("$.a").unwrap();
// "a" matches early, but "b" is malformed; a full parse rejects this.
assert_eq!(evaluate_path(r#"{"a":1,"b":}"#, &path), None);
}
#[test]
fn test_array_match_then_malformed_element_is_null() {
let path = parse_json_path("$[0]").unwrap();
assert_eq!(evaluate_path(r#"[1,,]"#, &path), None);
}
```
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -268,38 +269,151 @@ fn evaluate_path(json_str: &str, path: &ParsedPath) -> Option<String> {
1 => {
// Single wildcard match: Spark preserves JSON serialization format
// (strings keep their quotes, numbers don't)
- let s = serde_json::to_string(results[0]).ok()?;
- if s == "null" {
+ if results[0].is_null() {
None
} else {
- Some(s)
+ serde_json::to_string(results[0]).ok()
}
}
- _ => {
- // Multiple results: wrap in JSON array
- let arr = Value::Array(results.into_iter().cloned().collect());
- Some(arr.to_string())
+ // 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(),
+ }
+}
+
+/// 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> {
+ let mut de = serde_json::Deserializer::from_str(json_str);
+ let found = PathSeed { segments }.deserialize(&mut de).ok()?;
+ de.end().ok()?;
+ found
+}
+
+/// Deserializes the value at `segments`, discarding everything else.
+struct PathSeed<'a> {
+ segments: &'a [PathSegment],
+}
+
+impl<'de> DeserializeSeed<'de> for PathSeed<'_> {
+ type Value = Option<Value>;
+
+ fn deserialize<D: Deserializer<'de>>(self, deserializer: D) -> Result<Self::Value, D::Error> {
+ if self.segments.is_empty() {
+ return Value::deserialize(deserializer).map(Some);
}
+ deserializer.deserialize_any(SegmentVisitor {
+ segments: self.segments,
+ })
}
}
-/// Fast-path evaluation for paths without wildcards.
-/// Returns a reference to the matched value, or None if no match.
-fn evaluate_no_wildcard<'a>(value: &'a Value, segments: &[PathSegment]) -> Option<&'a Value> {
- if segments.is_empty() {
- return Some(value);
+/// Applies `segments[0]` to the value being visited. A value whose shape does
+/// not match the segment (an index into an object, say) is skipped and reported
+/// as no match rather than as an error, matching a lookup on a parsed document.
+struct SegmentVisitor<'a> {
+ segments: &'a [PathSegment],
+}
+
+impl<'de> Visitor<'de> for SegmentVisitor<'_> {
+ type Value = Option<Value>;
+
+ fn expecting(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ f.write_str("a JSON value")
}
- match &segments[0] {
- PathSegment::Field(name) => {
- let child = value.as_object()?.get(name)?;
- evaluate_no_wildcard(child, &segments[1..])
+ fn visit_bool<E>(self, _: bool) -> Result<Self::Value, E> {
+ Ok(None)
+ }
+
+ fn visit_i64<E>(self, _: i64) -> Result<Self::Value, E> {
+ Ok(None)
+ }
+
+ fn visit_u64<E>(self, _: u64) -> Result<Self::Value, E> {
+ Ok(None)
+ }
+
+ fn visit_f64<E>(self, _: f64) -> Result<Self::Value, E> {
+ Ok(None)
+ }
+
+ fn visit_str<E>(self, _: &str) -> Result<Self::Value, E> {
+ Ok(None)
+ }
+
+ fn visit_unit<E>(self) -> Result<Self::Value, E> {
+ Ok(None)
+ }
+
+ 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);
+ };
+
+ 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.
Review Comment:
`native/spark-expr/src/string_funcs/get_json_object.rs:360-361` visits every entry so a duplicated key "resolves to its last occurrence, as it would in a parsed object." That is correct for the old `serde_json` + `preserve_order` behavior (IndexMap overwrites the value, so `.get` returns the last), so this PR is faithful to the old Comet code. It is worth pinning with a test because the visit-every-entry loop is exactly what makes it work, and a future "break after match" optimization would silently flip it to first-wins:
```rust
#[test]
fn test_duplicate_key_last_wins() {
let path = parse_json_path("$.a").unwrap();
assert_eq!(evaluate_path(r#"{"a":1,"a":2}"#, &path), Some("2".to_string()));
}
```
Separately, note for the record that both old and new Comet diverge from Spark here. Spark's `GetJsonObjectEvaluator.evaluatePath` (`sql/catalyst/.../json/JsonExpressionEvalUtils.scala:478-488`) stops at the first matching field once `dirty` is set, so Spark returns the first occurrence, not the last. This is a pre-existing Comet bug, not something #4907 introduces, but the comment claiming parity with "a parsed object" is only true against serde, not against Spark. Consider filing a follow-up issue rather than expanding this PR.
##########
native/spark-expr/src/string_funcs/get_json_object.rs:
##########
@@ -268,38 +269,151 @@ fn evaluate_path(json_str: &str, path: &ParsedPath) -> Option<String> {
1 => {
// Single wildcard match: Spark preserves JSON serialization format
// (strings keep their quotes, numbers don't)
- let s = serde_json::to_string(results[0]).ok()?;
- if s == "null" {
+ if results[0].is_null() {
None
} else {
- Some(s)
+ serde_json::to_string(results[0]).ok()
}
}
- _ => {
- // Multiple results: wrap in JSON array
- let arr = Value::Array(results.into_iter().cloned().collect());
- Some(arr.to_string())
+ // 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(),
+ }
+}
+
+/// 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> {
+ let mut de = serde_json::Deserializer::from_str(json_str);
+ let found = PathSeed { segments }.deserialize(&mut de).ok()?;
+ de.end().ok()?;
+ found
+}
+
+/// Deserializes the value at `segments`, discarding everything else.
+struct PathSeed<'a> {
+ segments: &'a [PathSegment],
+}
+
+impl<'de> DeserializeSeed<'de> for PathSeed<'_> {
+ type Value = Option<Value>;
+
+ fn deserialize<D: Deserializer<'de>>(self, deserializer: D) -> Result<Self::Value, D::Error> {
+ if self.segments.is_empty() {
+ return Value::deserialize(deserializer).map(Some);
}
+ deserializer.deserialize_any(SegmentVisitor {
+ segments: self.segments,
+ })
}
}
-/// Fast-path evaluation for paths without wildcards.
-/// Returns a reference to the matched value, or None if no match.
-fn evaluate_no_wildcard<'a>(value: &'a Value, segments: &[PathSegment]) -> Option<&'a Value> {
- if segments.is_empty() {
- return Some(value);
+/// Applies `segments[0]` to the value being visited. A value whose shape does
+/// not match the segment (an index into an object, say) is skipped and reported
+/// as no match rather than as an error, matching a lookup on a parsed document.
+struct SegmentVisitor<'a> {
+ segments: &'a [PathSegment],
+}
+
+impl<'de> Visitor<'de> for SegmentVisitor<'_> {
Review Comment:
`native/spark-expr/src/string_funcs/get_json_object.rs` `SegmentVisitor::visit_unit` returns `Ok(None)`, which is what makes `$.a.b` on `{"a":null}` return null (recurse into null with a remaining segment yields no match). The old code got this from `Null.as_object()? -> None`. Parity holds, but there is no test for a null encountered mid-path (the existing `test_evaluate_null_value` only covers a terminal null). Add:
```rust
#[test]
fn test_null_midpath_is_null() {
let path = parse_json_path("$.a.b").unwrap();
assert_eq!(evaluate_path(r#"{"a":null}"#, &path), None);
}
```
--
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