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: [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]
