zhuqi-lucas commented on PR #21820: URL: https://github.com/apache/datafusion/pull/21820#issuecomment-4979096557
Design looks reasonable — hybrid split makes the perf trade-off explicit and benchmarks confirm no regression on the default path. Three things: - **Test coverage**: only Int32 / Float64 / (Int32+Utf8 multi-col) exercise the custom-hasher path. Would be good to add at least Dictionary, Struct, and Utf8View with long strings (to hit the `HAS_BUFFERS=true` const-generic branch). - **Parity test**: since the leaf loops are intentionally duplicated for perf, a test that runs both paths with a shared hash builder (e.g. `RandomState` wrapped as `BuildHasher`) and asserts equal output would catch drift cheaply. - **Nit**: `hash_null_with_hasher` names its parameter `mul_col`; the original `hash_null` uses `multi_col`. **Question**: what's the near-term consumer of `create_hashes_with_hasher`? #21428 has the motivation but no concrete caller — helpful to know if there's a follow-up PR that consumes this. -- 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]
