sunchao opened a new issue, #5994: URL: https://github.com/apache/datafusion-comet/issues/5994
### What is the problem the feature request solves? Comet cannot currently evaluate SQL `hash` and `xxhash64` natively when an input contains a decimal with declared precision above 18, including decimals nested in arrays, structs, and maps. Enabling native support would keep these expressions and their surrounding operators in native execution without JVM codegen dispatch. The gap remains on `main` at `58ab5f618e1e715dee06165424672fdd820cafe4`: - [The native wide-decimal branch](https://github.com/apache/datafusion-comet/blob/58ab5f618e1e715dee06165424672fdd820cafe4/native/spark-expr/src/hash_funcs/utils.rs#L201) hashes the unscaled `i128` using `to_le_bytes()`, a fixed-width 16-byte little-endian representation. - [Spark's decimal hashing](https://github.com/apache/spark/blob/v4.1.3/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala#L417) uses `toJavaBigDecimal().unscaledValue().toByteArray()` above precision 18: minimal-length signed two's-complement big-endian bytes. - [Comet's SQL admission guard](https://github.com/apache/datafusion-comet/blob/58ab5f618e1e715dee06165424672fdd820cafe4/spark/src/main/scala/org/apache/comet/serde/hash.scala#L136) rejects these types recursively, protecting SQL hash results from this mismatch. For example, an unscaled value of 128 in `DECIMAL(20,0)` is hashed by Spark as `00 80`; the existing native branch hashes `80` followed by fifteen zero bytes. The encoding choice depends on declared precision, even when a particular value fits in `i64`. Java `BigDecimal` is not intrinsically required to reproduce this representation. It can be computed directly from Arrow's unscaled integer in Rust. ### Describe the potential solution Implement Spark-compatible wide-decimal encoding in the native hash path, then admit the supported decimal inputs for both SQL hash functions. Preserve the existing unscaled-long hashing path for precision <=18. Acceptance criteria: - [ ] Both `hash` (Murmur3) and `xxhash64` match Spark for precision 19-38, including small values, values beyond 64 bits, precision-38 extremes, zero, negatives, and sign-padding/byte-length boundaries such as 127/128 and -128/-129. - [ ] Preserve null handling, multi-column hash chaining, and seeds. Test non-default seeds through the native/Catalyst APIs rather than treating an extra SQL hash argument as a seed. - [ ] Cover decimals in arrays, structs, and maps where Spark permits hashing, including null containers, null parent structs, and null decimal children. - [ ] End-to-end Spark parity tests require native execution and no JVM dispatcher activity, so fallback cannot make the tests pass silently. Retain narrow-decimal native controls. - [ ] Review and regression-test the shared native shuffle caller when changing the common encoding helper. Different partition numbers from Spark alone are not evidence of incorrect query results. - [ ] Add a focused benchmark against the existing fallback/dispatcher paths, with matched results and explicit routing checks. Report measured results without assuming a speedup. Keep dynamic `sha2`, `TimeType`, and enabling wide decimals for `approx_count_distinct` outside this issue's scope. ### Additional context - #1294 reported the original decimal hash mismatch. #1295 fixed the narrow-decimal path; #1325 closed the remaining issue by adding Spark fallback for precision >18. - #5581 and #5835 address these unsupported expressions through JVM codegen dispatch. This issue tracks the separate native implementation. - Coordinate with #5960 / #5103, which consolidate compatible `xxhash64` cases with upstream `SparkXxhash64`; native admission must be justified by Spark parity on the selected implementation. - #3079 discussed wide-decimal shuffle partitioning and was closed after noting that partition IDs need not match Spark. This is a native-support enhancement with an encoding-correctness prerequisite. The source analysis above does not establish an exposed wrong-result regression in the currently guarded SQL path, and no new native implementation or benchmark has been run for this issue. -- 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]
