jinjin1919 commented on issue #6984: URL: https://github.com/apache/datafusion-comet/issues/6984#issuecomment-6566849251
Update on the Map arm, as promised. I ran the set up and ran the test for Map arm refactoring. **Current state.** the four arms are at [L1134-L1249] They match what the issue describes: per pair, a loop for input without nulls and a null-aware loop, differing only in the downcast types and `.to_le_bytes()`. **Regression test (baseline behavior).** The `xxhash64_diff` tests (`map_utf8_int32` , `map_int32_utf8`, ...) cover all four pairs, only for xxhash64 expansion and only the null-aware loop. I ran the `xxhash64_diff` tests, 94 passes, 0 fails. I swapped the order of hash key vs hash value in the null-aware loop to test what would break the test. the result are as below: | Change | Existing tests | | --- | --- | | Hash the value before the key in the null-aware loop | `map_int32_int32` fails | | Same swap in the loop without nulls | all pass | | Drop the `map_array.is_null(row_idx)` check (null map that owns an entry) | all pass | So a refactor of these loops could change results and still go green. **A test that closes the gap.** I have a test locally that runs all four pairs through both hashes, with one row set that has no nulls and one with a null map that owns an entry and a null value. The expected values come from Spark 4.1.3 itself (`Murmur3Hash` / `XxHash64` evaluated on the same rows, seed 42), not from Comet's output. Current Comet matches Spark on all of them, and the test catches both of the changes above that the existing tests miss. **Plan for the refactor**, a `hash_map_entries!` helper modelled on `hash_list_primitive!`, taking the key/value arrays and a transform for each side, with the fast-path condition, key-before-value order and the separate null checks unchanged. I'd check it with the `hash` bench before and after (it only has `Utf8 -> Int32` maps today, so I will add an `Int32 -> Int32` case). @andygrove, I would like to take on map arm refactor issue and above is my plan for refactor. if all looks good. i will start implementing. -- 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]
