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

Spark's GetJsonObjectEvaluator returns the first duplicate-key occurrence that successfully resolves the remaining path. The native implementation previously kept the last occurrence. For example, get_json_object('{"a":1,"a":2}', '$.a') returned 2 natively while Spark returned 1.

What changes are included in this PR?

• Resolve named fields while streaming through JSON. Lock in the first successful occurrence, continue after a direct null or an unresolved nested path, and still consume the rest of the document so malformed JSON returns SQL NULL.
• Keep Spark's match flag separate from generator writes. Preserve array-reached nulls, nested and double wildcards, Raw/Quoted/Flatten output styles, and writes made by unmatched duplicate-key occurrences.
• Apply Jackson's numeric-token and 1,000-container nesting limits for Spark 3.5+; use a separate native function selected by the Spark 3.4 shim, whose Jackson version has no default limits. Skip the validation pass when the entire JSON is too short to exceed the numeric limit. Bounds-check string skipping so truncated escapes return NULL rather than panicking.
• Keep the common single result inline, and serialize terminal [*]/[*][*] results into one output buffer. This avoids per-leaf temporary strings while preserving writer counts, wrappers, flattening, and later parse errors.
• Render selected floating-point tokens with Spark's Java-style notation using a shared JSON formatter. Scalar results, nested objects and arrays, flattened leaves, and terminal wildcards use the same formatter without allocating a temporary string per wildcard leaf; for example, 0.0001 becomes 1.0E-4.

The native expression remains opt-in through spark.comet.expression.GetJsonObject.allowIncompatible=true; the default path uses Spark's JVM implementation. Known opt-in differences include single-quoted JSON, unescaped control characters, selected integers outside the 64-bit range or very long numbers (which can lose precision or fail during materialization), and duplicate keys inside a returned object or array: serde_json::Value collapses those keys during selected-subtree materialization, including for paths such as $.a. A few selected decimals can also differ at binary rounding or older-JDK formatting boundaries; the formatter fixes common notation differences but does not claim byte-exact output for every floating-point value.

There is also an unavoidable near-limit floating-number difference. Jackson allocates String-parser buffers by input length and recycles larger buffers on the same JVM thread. The same 1,001-digit float can therefore be accepted or rejected by Spark depending on that prior state. The native function cannot infer the state from its JSON input; it applies a deterministic 1,000-digit rule, preserving the input-determined leading-zero and EOF cases, and conservatively rejects other over-limit tokens. This replaces the earlier, incorrect fixed 4,000-character buffer approximation.

How are these changes tested?

• Rust tests cover duplicate-key first-successful-match behavior, null and wildcard styles, malformed input, version-specific numeric and nesting limits, the terminal-wildcard buffer path, and Java-style float output through scalar, nested, and wildcard paths. After syncing with upstream main, the full expression crate has 1,033 passing unit tests, 1 passing integration test, and 7 passing registration tests; native build, Clippy with -D warnings, and cargo fmt --check pass.
• The get_json_object.sql fixture compares native results with Spark for both dictionary settings, including 0.0001, 12345678.9, returned containers, and wildcards. Focused runs on the merged branch pass on Spark 3.4, 3.5, 4.0, 4.1, and 4.2; Scala style and formatting checks pass in those builds.
• The Spark 4.1.3 SQL Core local CI shard completed with 12,858 passed, 3 failed, and 0 errors. All three failures were Spark UI REST tests receiving HTTP 502 from this environment's proxy for 10.255.255.254:4040; the same three tests passed when rerun with the UI bound to 127.0.0.1 and proxy variables unset. An earlier Catalyst shard also passed: 8,507 passed, 0 failed, 0 errors (5 ignored).
• Criterion comparison on the merged branch: 1,000-number terminal wildcards improved from 5.57 to about 1.92 ms per 64-row batch (about 66%); 1,000-string wildcards from 6.62 to about 2.98 ms (about 55%). Short-record field extraction improved from about 3.64 to 2.28 ms per 8,192-row batch after avoiding the redundant validation pass; the large-skipped-string case measures about 0.83 ms per batch.
• The new 1,000-float terminal-wildcard stress benchmark completes in about 9.15 ms per 64-row batch with the Java-style formatter; it is retained for future performance comparisons.

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

[andygrove]

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]

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]

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.

[sunchao]

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

[u70b3]

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.

[sunchao]

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

[u70b3]

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]

x

[sunchao]

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.

[sunchao]

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

[u70b3]

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.

[sunchao]

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

[u70b3]

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.

[rich7420]

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.

[rich7420]

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.

[sunchao]

Four P2 findings remain at head a1fd4c9. I would hold approval. These affect the opt-in native implementation; default JVM execution is unaffected.

1. Malformed JSON can panic instead of returning NULL. The escape-handling branch advances without ensuring the next slice stays within bounds. This remains in get_json_object.rs:319. Confirmed by source inspection; already reported in an unresolved thread.

2. The unconditional numeric limit regresses Spark 3.4. An object containing a:1 and an unselected 1,001-digit integer returns 1 for $.a on Spark 3.4.3 and the PR base, but NULL on this head. The limit at line 326 needs version-aware behavior. Fresh evaluator comparisons confirm the existing report.

3. Numeric counting still disagrees with Spark 4.1 at reader-buffer boundaries. With 3,000 padding characters before an unselected 1. followed by 1,000 fractional digits, Spark 4.1.3 and the base return 1; head returns NULL. Jackson’s fast and refill paths count absent components differently, which the digit sum misses. Reproduced through actual Spark-to-Comet execution with constant and column paths.

4. Result construction adds substantial allocation overhead. PathResult construction, parent appends, and final joining increase small-string extraction from 1 to 4 allocations, and nested extraction from 1 to 6. Fresh component benchmarks measured 1.60×, 1.75×, and 2.03× runtime for small strings, nested strings, and a 1,000-number wildcard respectively. Move owned singleton results and avoid intermediate serialization buffers while retaining the required output-style state.

Validation: 47 native tests passed, and the Spark 4.1 SQL fixture passed both dictionary configurations. Numeric comparisons and the 46,988-case semantic comparison were rerun. Full native rebuilds hit disk exhaustion; successful execution checks used the existing library built from this same head. Performance figures are evaluator measurements, not whole-query timings.

Earlier wrapper and bytewise string-scan findings are fixed. Comet CI and CodeQL still require workflow approval.

[sunchao]

Re-reviewed d291d19879acea8382ead3a625181e8e491f4686 against base 1c25b492bfb5d4615c7838cf24ec4da867263034. Four P2 issues remain in the opt-in native implementation, detailed inline: fixed-size Jackson buffer emulation rejects values Spark accepts, streaming wildcards bypass Spark's nesting limit, short documents pay unnecessary numeric-validation overhead, and dense wildcard serialization remains substantially slower than base.

The streaming design and separate output-style/match state are justified. The previous truncated-string panic, Spark 3.4 numeric-limit regression, and singleton-allocation issue are fixed. No new Spark operator fallback was introduced. The default JVM codegen dispatcher and spark.comet.expression.GetJsonObject.allowIncompatible=true native opt-in boundary remain unchanged.

Validation: 50 focused native tests passed after rebuilding the expression crate from this head. Both correctness findings reproduce through the compiled scalar/scalar, column/scalar, and column/column entry points, compared with actual versioned Spark evaluators and the exact base evaluator. The recycled-buffer behavior also reproduces in a real Spark 4.1.3 JSON Dataset query with whole-stage codegen disabled and enabled. A 100,201-case Spark 4.1.3 differential corpus found no additional actionable ordinary duplicate-key/null/wildcard issue; that count is comparisons, not universal compatibility passes. Repeated alternating component benchmarks corroborate the performance findings.

Validation limits: performance figures are evaluator timings, not whole-query timings. Full current-head Spark-to-Comet SQL integration and the full supported-version matrix were not rerun locally because disk was constrained. The author's reported broader runs are not counted as independently reproduced results.

Current-head Comet CI, CodeQL, and PR title check still report action_required; labeling passed. Holding approval pending the four findings below.

[sunchao]

[P2] Account for recycled Jackson buffers in numeric validation

This assumes every reader buffer has 4,000 UTF-16 units, but Jackson can reuse a larger buffer allocated by an earlier String parser. For {"a":1,"pad":"<5000 x characters>","n":1.<1000 zeros>}, $.a and $.n return 1 and 1.0 on the base and actual Spark with a recycled 6,000-character buffer. This head returns NULL for both, including through the compiled scalar and both column entry points.

I reproduced the larger-buffer behavior on Spark 3.5.9, 4.0.4, and 4.1.3 evaluators. It also occurs in ordinary Spark 4.1.3 SQL: read a 6,000-character JSON Dataset row, build this JSON from a nonconstant input column, then apply get_json_object. It reproduces with whole-stage codegen disabled and enabled. JsonFactory allocates the String parser buffer by input length, and BufferRecycler retains larger buffers.

Could we revisit this fixed-buffer compatibility model and cover the JSON Dataset pipeline? A fixed modulo cannot recover the JVM recycler state from the JSON text. An explicit compatibility policy for these rare long-number inputs would also be clearer than claiming reader-boundary parity.

[sunchao]

[P2] Preserve Spark's nesting limit when streaming wildcard paths

For $[*].a, construct the valid JSON as '[{"a":1,"skip":' + '[' * 999 + '0' + ']' * 999 + '}]'. Its total nesting depth is 1,001. Spark 3.5.9, 4.0.4, 4.1.3, and the PR base return SQL NULL, while this head returns 1. I reproduced the head result through the compiled scalar and both column entry points. Reducing the inner arrays to 998 makes Spark and head return 1; either field order reproduces the boundary.

The new wildcard traversal skips unrelated subtrees through IgnoredAny, which does not enforce Jackson's depth constraint. This extends a pre-existing nonwildcard limitation to wildcard paths that the base rejected.

Could we enforce the applicable Spark version's nesting limit while skipping subtrees and add both sides of this boundary? Spark 3.4.3 accepts these documents, so the check needs to be version-aware. Restoring serde's older 128-level limit would be too restrictive.

[sunchao]

[P2] Skip numeric validation for inputs too short to exceed the limit

This scans every document before extraction on Spark 3.5+, even when the entire input is at most 1,000 bytes and therefore cannot contain more than 1,000 ASCII numeric digits.

For a representative 198-byte record with path $.name, an independent five-round alternating exact-source benchmark measured 338.54 ns on base versus 547.29 ns on head. Adding only json_str.len() > 1000 before this scan reduced the result to 356.60 ns, with identical outputs. Two separate seven-round runs corroborated the slowdown. Paths were parsed once and inputs/results were black-boxed; these are evaluator timings, not whole-query timings.

Could we add this length guard? Serde still validates the document's syntax. The earlier bytewise 64 KiB string-scan issue is fixed; this is remaining avoidable work on ordinary short records.

[sunchao]

[P2] Avoid per-leaf temporary serialization for dense wildcard results

Each selected wildcard leaf is serialized into its own String here, then accumulated and copied into the wrapper at line 528. Repeated alternating exact-source benchmarks against the PR base show 1,000-number wildcard extraction at about 1.8-2.1x base runtime, and 1,000-string extraction at 1.65-1.69x. An independent five-round check measured 54.02 us to 99.57 us for numbers and 94.23 us to 157.10 us for strings. The regression persists with numeric validation disabled, so it is separate from the pre-scan issue.

The singleton ownership fix works, and numeric-wildcard allocation counts have improved. This finding concerns the remaining serialization/copy CPU cost. Sparse wildcard objects improve through selective traversal, so keeping that behavior matters.

Could we add a common terminal-wildcard path that writes directly into an output buffer while preserving the existing per-level styles, match counts, and duplicate-field side effects? These measurements use cached parsed paths and black-boxed inputs/results and establish evaluator regressions, not whole-query timings.

[andygrove]

This is a light fully automated review since there are so many PRs open.

Selected floats come back in a different notation from Spark. This predates the PR, but it now lives in the new PathResult::write. Spark's generator copies a float token through double (Jackson's _copyCurrentFloatValue calls writeNumber(p.getDoubleValue()), which prints with Double.toString), while value.to_string() at native/spark-expr/src/string_funcs/get_json_object.rs:510 and the flatten leaves at line 559 use serde_json's shortest form. So get_json_object('{"a":0.0001}', '$.a') returns 1.0E-4 in Spark and 0.0001 natively, and 12345678.9 comes back from Spark as 1.23456789E7. Floats nested inside a returned object or array differ the same way, since copyCurrentStructure copies them through the same method. Unlike the 1,000-digit cases, these are ordinary values. Neither this nor the "selected very large numeric values" gap from the description is in getIncompatibleReasons() at spark/src/main/scala/org/apache/comet/serde/strings.scala:699, which is what the generated compatibility page is built from. Could the output go through a serde_json::ser::Formatter whose write_f64 uses the existing write_java_float_string in native/spark-expr/src/conversion_funcs/numeric.rs, with a few of these values added to get_json_object.sql? If that is more than this PR should take on, an issue plus a reasons entry and these queries behind ignore(<issue link>) would at least record the gap where the next person will look.