fix: get_json_object returns first value for duplicate keys to match Spark

[u70b3]

Which issue does this PR close?

Closes #4947.

Rationale for this change

For a JSON object with duplicate keys, Spark's GetJsonObjectEvaluator keeps the first occurrence that produces a value, but Comet's native implementation resolved the key to its last occurrence (matching serde_json's overwrite semantics rather than Spark's):

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 successful match for a key and consumes later occurrences via IgnoredAny instead of re-parsing them with PathSeed:

• The lock is keyed on a successful match, not on merely seeing the key. This mirrors Spark's GetJsonObjectEvaluator.evaluatePath, where a named field whose value is JSON null — or whose subtree does not resolve the rest of the path — does not set dirty, and evaluation continues to later duplicate keys. For example, {"a":null,"a":2} / $.a and {"a":{"x":1},"a":{"b":2}} / $.a.b both return 2 in Spark.
• All entries are still visited, so the existing malformed/trailing-garbage rejection behavior is unchanged.

The PR also includes two supporting changes in the same file:

• Option<Value> is replaced by a PathResult carrying the matched values plus a separate matched flag, and PathSeed gains a reject_direct_null flag capturing Spark's rule that a JSON null directly below a named field is not a match, while a null reached through array traversal is a match and serializes as the text null (e.g. {"a":[null]} / $.a[0] now returns the string null instead of SQL NULL, matching Spark's copyCurrentStructure).
• Wildcard paths are now evaluated with the same streaming seed parser instead of materializing the whole document with serde_json::from_str. A single wildcard match on a null also serializes as null text, matching Spark.
• Two consecutive subscript wildcards [*][*] parse to a dedicated DoubleWildcard segment, matching Spark's "non-structure preserving double wildcard" case in evaluatePath: the remaining path applies to the outer array's elements themselves (in flatten style, splicing array leaves recursively), and the collected matches are always wrapped in a single array, even a single one. Previously [*][*] was treated as two independent wildcards, which descended into the inner arrays instead — so {"a":[[{"b":1}]],"a":null} / $.a[*][*].b returned 1 where Spark returns NULL (the first a's outer element is an array and cannot match .b, and the second a is null).

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_successful_match_wins covering Spark's continue-on-null / continue-on-missing-subpath semantics ({"a":{"x":1},"a":{"b":2}} / $.a.b -> 2, {"a":null,"a":{"b":2}} / $.a.b -> 2, {"a":null,"a":2} / $.a -> 2, {"a":{"b":null,"b":2}} / $.a.b -> 2).
• Added test_duplicate_key_first_successful_match_wins_with_wildcard and test_duplicate_key_null_reached_through_array_locks_match.
• Added test_null_reached_through_array_serializes_as_null_text for the non-duplicate-key case ({"a":[null]} / $.a[0] and $.a[*] -> null text, {"a":[null,1]} / $.a[*] -> [null,1]).
• Added test_duplicate_key_double_wildcard_match_decision plus five more unit tests pinning the [*][*] flatten semantics (one-level flatten mirroring Spark's own $.store.basket[*][*] case, single match staying wrapped, empty-flatten no-match, recursive flatten).
• Added [*][*] queries — including the reviewer-reported {"a":[[{"b":1}]],"a":null} / $.a[*][*].b case — to spark/src/test/resources/sql-tests/expressions/string/get_json_object.sql, so they are validated against Spark itself. CometSqlFileTestSuite passes locally on Spark 4.1.3 (463 tests).
• Full datafusion-comet-spark-expr suite passes, 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.

[andygrove]

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:

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:

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:

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.

[u70b3]

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.

[rich7420]

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?

[u70b3]

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.