sunchao commented on PR #4971: URL: https://github.com/apache/datafusion-comet/pull/4971#issuecomment-5902094332
Found **one new P2** in the five-agent review of `87790224` against base `222609d8`. **[P2] Avoid Revalidating Serialized UTF-8** [get_json_object.rs:542](https://github.com/apache/datafusion-comet/blob/87790224c8649404448c2a119b5b75ef93056785/native/spark-expr/src/string_funcs/get_json_object.rs#L542) and [line 645](https://github.com/apache/datafusion-comet/blob/87790224c8649404448c2a119b5b75ef93056785/native/spark-expr/src/string_funcs/get_json_object.rs#L645) rescan output that the serializer already guarantees is valid UTF-8. For `$.a[*]` returning a 65,535-byte CJK string, repeated benchmarks measured **56.87 us on base versus 100.52 us on head: 77% slower**. Removing only redundant validation reduced this to **58.45 us**, with identical output, also verified against Spark. These inputs contain no floats. Could we preserve the serializer’s documented UTF-8 invariant when constructing the final string and add a non-ASCII output benchmark? These are evaluator timings, not whole-query measurements. **Other Conclusions** - The previous four findings are addressed, including the explicitly documented numeric-limit compatibility policy. - No new actionable correctness issue or unnecessary abstraction was verified. - No new Spark operator fallback. Native execution remains opt-in; default JVM dispatch is unchanged. - Float-heavy arrays measured **1.8–1.9x slower** with the Java-style formatter. This is a separate measured compatibility tradeoff. **Validation** - Passed **1,033 unit tests**, including 53 JSON tests, plus 8 integration/registration tests. - Spark 4.1/JDK 17 SQL fixture passed both dictionary configurations using the freshly built native library. - Checked 155,663 traversal cases, 4,399 numeric cases, 240 edge cases, and 96 compiled-entry-point comparisons. Existing incompatibilities remain. - Full upstream Spark suites/version matrix were not rerun. [Comet CI](https://github.com/apache/datafusion-comet/actions/runs/36542733719), CodeQL, and title checks remain `action_required`. I haven’t posted this new finding. -- 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]
