sunchao commented on PR #4971: URL: https://github.com/apache/datafusion-comet/pull/4971#issuecomment-5804035272
**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](https://github.com/apache/datafusion-comet/blob/a1fd4c955a6ebbe3ac9551ff49a293c26f4cd4fc/native/spark-expr/src/string_funcs/get_json_object.rs#L319). 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](https://github.com/apache/datafusion-comet/blob/a1fd4c955a6ebbe3ac9551ff49a293c26f4cd4fc/native/spark-expr/src/string_funcs/get_json_object.rs#L326) 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](https://github.com/apache/datafusion-comet/blob/a1fd4c955a6ebbe3ac9551ff49a293c26f4cd4fc/native/spark-expr/src/string_funcs/get_json_object.rs#L381) misses. Reproduced through actual Spark-to-Comet execution with constant and column paths. 4. **Result construction adds substantial allocation overhead.** [PathResult construction](https://github.com/apache/datafusion-comet/blob/a1fd4c955a6ebbe3ac9551ff49a293c26f4cd4fc/native/spark-expr/src/string_funcs/get_json_object.rs#L459), 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](https://github.com/apache/datafusion-comet/actions/runs/35499100063) and [CodeQL](https://github.com/apache/datafusion-comet/actions/runs/35499099564) still require workflow approval. -- 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]
