Skip to content

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

Open
u70b3 wants to merge 6 commits into
apache:mainfrom
u70b3:fix/json-dup-key-first-wins
Open

u70b3 wants to merge 6 commits into
apache:mainfrom
u70b3:fix/json-dup-key-first-wins

Conversation

@u70b3

@u70b3 u70b3 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

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).

  • PathSegment::Wildcard is split by form: [*] becomes SubscriptWildcard and .*/['*'] become ChildWildcard. Spark's evaluator has no reachable arm for the child-wildcard form (its parser emits a bare wildcard instruction that no dispatch case consumes), so those paths now return null for every document instead of iterating an array.

  • Spark's WriteStyle machinery is ported end to end. A Style {Raw, Quoted, Flatten} value propagates through the evaluation the way evaluatePath threads its style parameter, and each wildcard arm makes its own wrapper decision: an index immediately followed by [*] switches to Quoted style (whose wildcard keeps its array wrapper even for a single match), and wildcards nested below another wildcard run in Quoted style and stay wrapped. Previously the wrapper decision was made once at the top from the flat list of matches, which could not represent per-level nesting: [[[[[[[1]]]]]]] / $[0][*][0][*][*] returned [1] where Spark returns [[1]], and $.store.basket[0][*].b / $.a[*].b[*] returned "y" / [1,2,3] where Spark returns ["y"] / [[1,2],[3]] (the latter two were pre-existing on main).

  • PathResult is now modeled on Spark's generator protocol: a list of rendered fragment writes plus the dirty flag. This reproduces behaviors a value-tree cannot express — the Quoted and double-wildcard arms write their brackets even when nothing inside matched, and Spark's generator keeps those bytes, so an unmatched duplicate-key occurrence followed by a matching one yields output like [] [1] (root-level writes separated by a space, as Jackson does). A Raw-style string leaf is written unquoted, which replaces the old has_wildcard heuristic at the top.

  • A pre-parse scan rejects documents containing numbers whose digit count exceeds 1000, mirroring the counters of jackson-core 2.21.2's StreamReadConstraints (the version Spark 4.1.3 pins): the sign and 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, except when both a fraction and an exponent are present), fraction and exponent digit counts. Such numbers return null anywhere in the document — including values the path never selects — which matches Spark; serde_json's IgnoredAny skip imposes no such limit, and serde's sealed Read trait gives no way to hook the token stream, so a scan is the only enforcement point. String bodies are skipped inline or with memchr2 (the same approach as serde_json's own ignore_str), so large unselected strings cost about one extra skip-pass; a large_skipped_string criterion benchmark guards the case.

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.
  • Added write-style queries to the same SQL file: $[0][*] (wrapper kept after an index), $[0][*][0][*][*] (review regression), $.store.basket[0][*].b and $.a[*].b[*] (pre-existing gaps, now asserted positively), $[*][*][*] (flatten flowing into a remaining wildcard), the never-matching .*/['*'] forms, and the unmatched-duplicate-key wrapper output [] [1].
  • Added numeric-length boundary queries via repeat(...): 1000/1001-digit integers, signed 1000/1001, fraction-only and exponent-only boundaries on both sides, and the leading-zero shapes (0.<1000 digits>, 0e<1000 digits>).
  • Added unit tests test_index_then_wildcard_keeps_wrapper, test_wildcard_below_wildcard_keeps_inner_wrapper, test_oversized_number_in_skipped_field, test_child_wildcard_never_matches, test_wildcard_unmatched_writes_are_kept, and test_triple_wildcard_flatten.
  • Differential validation: a 600-case battery (20 documents × 30 paths) plus a 69-case numeric-boundary battery (signed, fractional, exponent and leading-zero shapes, string-content and escaped-quote controls) were evaluated with Spark 4.1.3's own GetJsonObjectEvaluator (driven directly from the 4.1.3 catalyst jar) and compared against this implementation. The only differences are the pre-existing value-materialization cases below. The 4 mismatches are all $-on-duplicate-keys documents, where serde_json's Value materialization collapses the duplicate keys that Spark's token copy preserves — a pre-existing limitation of value materialization, unchanged by this PR (the same holds for selected numbers of 309–1000 digits, which overflow f64 during materialization but are copied verbatim by Spark).
  • 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.

@u70b3
u70b3 force-pushed the fix/json-dup-key-first-wins branch 3 times, most recently from 6f73bf5 to 361a314 Compare July 22, 2026 12:01
@u70b3
u70b3 force-pushed the fix/json-dup-key-first-wins branch from 7037198 to c36c06c Compare July 31, 2026 05:09
@u70b3
u70b3 force-pushed the fix/json-dup-key-first-wins branch 2 times, most recently from c5b82da to d38a346 Compare August 27, 2026 07:59
@andygrove

Copy link
Copy Markdown
Member

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

u70b3 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

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.

@u70b3
u70b3 force-pushed the fix/json-dup-key-first-wins branch 2 times, most recently from a8b0928 to 1a7d25d Compare September 4, 2026 02:41
@andygrove andygrove added bug Something isn't working correctness area:expressions Expression evaluation json expressions labels Sep 6, 2026
@u70b3
u70b3 force-pushed the fix/json-dup-key-first-wins branch from 1a7d25d to 8db201e Compare September 8, 2026 06:55
while let Some(matched) = map.next_key_seed(KeySeed(name))? {
if matched {
found = map.next_value_seed(PathSeed {
if matched && !found.matched {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were right on the lock semantics and I was wrong. evaluatePath sets dirty only when something is written, and a named field whose value is VALUE_NULL returns false, so continuing on to a later duplicate is Spark's behaviour rather than a divergence from it. The updated description matches the code now.

I built the branch and checked the [*][*] fix against the expectations in Spark's own JsonExpressionsSuite rather than re-deriving them. $.store.basket[*][*], [*][0], [0][*], [*].category, [*].isbn, [*].reader and the non-existent-key cases all match. The parse-time merge is faithful too. Spark's parser emits Subscript :: Wildcard for each [*] and a bare Wildcard for .* and ['*'], so only the subscript form pairs, and evaluatePath pairs them greedily left to right in the same way a left-to-right merge does.

Two cases still differ, and I confirmed they differ identically on main at 58ab5f6, so neither comes from this PR:

$.store.basket[0][*].b   Spark ["y"]         Comet "y"
$.a[*].b[*]              Spark [[1,2],[3]]   Comet [1,2,3]

The first is a case in Spark's own suite. The cause is that Spark switches RawStyle to QuotedStyle on entering an array wildcard, and the wildcard arm under QuotedStyle wraps unconditionally, so only the outermost wildcard ever gets the single-match unwrap. evaluate_path collects into a flat Vec and decides the wrapping once at the top, which cannot represent that nesting. It is a redesign rather than a patch, so I would rather it did not ride along here. Could you file an issue for it and add those two queries to get_json_object.sql behind ignore(<issue link>), so the gap is recorded where the next person will look?

Approving. A pre-existing gap should not hold up a change that makes duplicate keys and [*][*] correct.

@u70b3

u70b3 commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

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!

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 7f54d2ca7277d10023e3c913c4a2729b9b6673b0 against base bb9e74020adc228e486f6f4d0fa68292b30bff31. Found two new correctness regressions in the opt-in native get_json_object implementation, detailed inline: a nested wildcard loses an array dimension, and wildcard extraction accepts an oversized skipped numeric token that Spark rejects. The default JVM-dispatched path is unaffected.

Validation: all 41 focused native tests passed with cargo test -p datafusion-comet-spark-expr get_json_object --locked --offline. Both findings were independently reproduced using the compiled head UDF in scalar/scalar, column/scalar, and column/column modes, compared with Spark 4.1.3's actual GetJsonObjectEvaluator and the exact source-extracted base evaluator. The full Comet JVM integration suite was not run locally, so these checks do not establish end-to-end Spark/Comet SQL execution.

Current-head Comet CI and CodeQL await workflow approval. The failed Spark 4.1 job in the earlier CI run stopped during dependency resolution with Connection reset downloading org.ow2.asm:asm:9.9; its merge tree differs from the reviewed head. Holding approval while the two reproduced regressions remain.

Comment on lines +464 to +466
if result.matched {
found.matched = true;
found.values.append(&mut result.values);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7099e9d — 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.

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)?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7099e9d. 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.

@u70b3

u70b3 commented Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

x

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed 7099e9d2d657aa24267f5d34111107be66bc6dac against base bb9e74020adc228e486f6f4d0fa68292b30bff31 and the previously reviewed 7f54d2ca7277d10023e3c913c4a2729b9b6673b0. Both earlier concrete reproductions now match Spark: the nested wildcard retains [[1]], and the skipped positive 1,001-digit number returns SQL NULL. Two new issues in the numeric pre-scan remain, detailed inline: valid signed/fractional numbers are rejected at the length boundary, and scanning quoted strings byte by byte causes a substantial evaluator regression. These affect the opt-in native implementation (spark.comet.expression.GetJsonObject.allowIncompatible=true); the default JVM-dispatched implementation is unchanged.

Validation: all 47 focused native tests passed with cargo test -p datafusion-comet-spark-expr get_json_object --locked --offline. The new numeric counterexamples were independently checked against Spark 4.0.4 and 4.1.3, the exact base/prior/head evaluator sources, and the freshly compiled head UDF in scalar/scalar, column/scalar, and column/column modes. Optimized component benchmarks used cached parsed paths, black-box inputs/results, and alternating revision order; a separate harness reproduced the slowdown and isolated the pre-scan. These are evaluator timings, not whole-query timings. The full Comet JVM integration suite was not run locally, so the direct Spark evaluator/native UDF checks do not establish end-to-end Spark/Comet SQL execution.

Current-head Comet CI, CodeQL, and PR title check still await workflow approval; labeling passed. Holding approval pending the two findings below.

Comment on lines +327 to +328
if i - start > MAX_NUMBER_LEN {
return true;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +340 to 342
if has_oversized_number(json_str) {
return None;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

…h Spark

Spark's GetJsonObjectEvaluator stops at the first matching field, but
Comet's SegmentVisitor.visit_map kept overwriting the result, resolving
a duplicated key to its last occurrence. Lock in the first match (even
when the subpath misses, per Spark semantics) and skip later occurrences
while still consuming all entries to validate the document.

Closes apache#4947
Spark serializes a null reached through array traversal as the JSON
text null (copyCurrentStructure), unlike a null directly under a named
field, which is not a match. Add non-duplicate-key coverage for the
$.a[0] and $.a[*] cases.
Spark's evaluatePath consumes two consecutive subscript wildcards as a
single non-structure-preserving step: the remaining path applies to the
outer array's elements in flatten style, and the collected matches are
always wrapped in one array, even a single one. Treating `[*][*]` as two
independent wildcards descended into the inner arrays instead, so
`{"a":[[{"b":1}]],"a":null}` with `$.a[*][*].b` returned 1 where Spark
and the pre-change native UDF return NULL: the first `a` misses (its
outer element is an array with no field `b`) and the second is null.

Parse `[*][*]` into a dedicated DoubleWildcard segment, propagate
Spark's flatten style to leaf values (splicing array leaves
recursively), and add Rust unit tests plus SQL-file cases.
…cards

Port Spark's WriteStyle machinery (Raw/Quoted/Flatten) from
JsonExpressionEvalUtils so wildcard wrappers are decided per wildcard
level rather than once at the top:

- an index immediately followed by `[*]` switches to Quoted style, whose
  wildcard keeps its array wrapper even for a single match (review
  regression: `$[0][*][0][*][*]` lost an array dimension)
- wildcards nested below another wildcard stay wrapped (closes the
  `$.store.basket[0][*].b` and `$.a[*].b[*]` gaps; both now asserted in
  the SQL tests)
- results are modeled on Spark's generator protocol (fragment writes +
  dirty flag), which also reproduces the unmatched-duplicate-key wrapper
  output such as `[] [1]`
- `.*`/`['*']` wildcards never match, matching Spark (no reachable arm)
- number tokens over 1000 characters anywhere in the document return
  null, mirroring Jackson's StreamReadConstraints (review regression:
  previously accepted in skipped fields)

Validated with a 600-case differential against Spark 4.1.3's
GetJsonObjectEvaluator; the only remaining differences are the
pre-existing `$`-on-duplicate-keys materialization gap.
…chr speed

The pre-parse scan for Jackson's 1000-digit numeric constraint now counts
the way jackson-core 2.21.2 does: the sign and decimal point do not
count, integers are limited by their digit count, and floats by the sum
of integer-part (a lone leading zero counts as zero digits, except when
both a fraction and an exponent are present, where it counts as one),
fraction and exponent digit counts. Previously the scan counted raw
token bytes, so a skipped -<1000 digits> or 0.<999 digits> value nulled
out an otherwise successful extraction.

String bodies are no longer walked byte by byte: short bodies are
scanned inline and long bodies use memchr2, the same approach as
serde_json's own ignore_str. On a 64 KiB unselected string field the
evaluator goes from ~11.6x slower than base to within a few percent; a
new large_skipped_string criterion benchmark guards the case.

Validated with a 69-case boundary battery (signed, fractional, exponent
and leading-zero shapes) against Spark 4.1.3, plus the existing
600-case differential, which shows no new divergences.
@u70b3
u70b3 force-pushed the fix/json-dup-key-first-wins branch from 38429ff to a1fd4c9 Compare September 20, 2026 08:16
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

fn has_oversized_number(json: &str) -> bool {
const MAX_NUMBER_DIGITS: usize = 1000;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, which has no such limit. Please preserve the version-specific behavior and add a Spark 3.4 regression while retaining the 4.1 validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:expressions Expression evaluation bug Something isn't working correctness json expressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Native implementation of get_json_object returns last value for duplicate keys, Spark returns first

4 participants