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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
