zhangfengcdt commented on PR #6599: URL: https://github.com/apache/datafusion-comet/pull/6599#issuecomment-5998084734
> Summary > > * Prior state and problem: Float-bearing `array_contains` used Spark’s dispatcher by default. Opting into native execution exposed incorrect signed-zero and NaN comparisons. > * Design approach: Add `spark_array_contains` with Spark-compatible float equality and enable it for flat and nested float-bearing elements. > * Correctness / compatibility analysis: Checked Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0 sources. Float equality and ordinary null handling match. Two existing P2 reports remain reproducible: [collated struct comparisons](https://github.com/apache/datafusion-comet/pull/6599#discussion_r4178402154) return `false` where Spark returns `true`, and [throwing lookup expressions](https://github.com/apache/datafusion-comet/pull/6599#discussion_r4178402158) execute for null arrays, aborting queries where Spark returns null. > * Key design decisions: Reusing `spark_equality` and extracting `set_bits_before` keeps the implementation localized. Constant float lookups scan flattened values, while per-row lookups stop at a match. The helper extraction preserves `array_remove` behavior. > * Implementation sketch: Scala selects the registered Rust UDF. Native tests, SQL fixtures, routing assertions and compatibility documentation accompany the change. > * Behavioral changes worth calling out: Compared with `branch-1.1` at `992c806a7e38c2e88bd018aa5774164b0850e1fa`, native routing and corrected opt-in float equality are intended. The correctness regressions are unintended. The existing [performance review](https://github.com/apache/datafusion-comet/pull/6599#pullrequestreview-5407332548) also remains unresolved: its struct-array workload measured 3,831–4,060 ms here versus 2,591–2,739 ms with the dispatcher. Those measurements were not rerun in this pass. > * Suggested improvements: Address the existing requests to preserve dispatch for nested elements and unsafe eager lookup evaluation, with regression coverage. No additional introduced P1/P2 issues found within this review beyond the existing unresolved reports. The existing request-changes concerns remain valid. > > Reviewed all 11 changed files at `46faab056ba96f002559ae405a10e5a3362ddcf6` against `fe5c41aab1159ee8ea995da90b48061a905e92bf`. Confirmed non-draft status. Read existing reviews and threads. Routed skills: `review-comet-pr` and `review-comet-expression-pr`. > > Exact-head CI: 25 passed, 15 skipped, no failed or running checks. Rust tests, Linux Spark 4.1 suites and TPC-H/TPC-DS verification passed. Spark SQL suites, macOS and benchmark checks were skipped. > > Validation: five `array_contains` tests, seven `array_remove` tests and three disposable native probes passed. The probes checked sliced buffers and reproduced both existing correctness regressions. Spark 4.1.3 interpreted and generated Catalyst probes confirmed the expected results. No fresh end-to-end Comet SQL, full Spark SQL suite or performance run was completed. Disposable project tests were removed, and the checkout is clean. Thanks @andygrove @sunchao for the review. I have made some changes to address the comments: 1. Only flat `FLOAT`/`DOUBLE` arrays route to `spark_array_contains` now. Nested float elements stay on the dispatcher. 2. Over a nullable array, a value other than a literal or column read stays on the dispatcher, as `CometArrayJoin.orderInsensitive` does. 3. Added `array_contains_eager_eval.sql` (ANSI cast over a null array) and `array_contains_collation.sql` (your collated struct query), both asserting dispatch. 4. Updated `expressions.md`, the `array_contains.sql` comments, the `CometMapExpressionSuite`comment, and "Both expressions" in `floating-point.md`. The map test's `allowIncompatible` is meaningful again since its key is a cast, so the comment now says it lifts the eager-evaluation guard. Also I updated the pr title to be more accurate. -- 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]
