andygrove commented on PR #5150: URL: https://github.com/apache/datafusion-comet/pull/5150#issuecomment-5134802629
Follow-up commit with benchmark numbers, since "does this change performance?" is the obvious question for a change to per-row cast kernels. Measured with `cast_from_string` (criterion, this branch vs `apache/main`, same machine): | Cast | Change | | --- | --- | | string → boolean | **63-67% faster** | | string → boolean (space-padded) | **65-67% faster** | | string → double | **12-13% faster** | | string → double (space-padded) | **20-21% faster** | | string → decimal | **3-7% faster** | | string → byte/short/int/long | unchanged (within ±3% run-to-run noise) | The boolean win is from dropping the per-row `to_ascii_lowercase()` allocation; the float/decimal wins are from replacing `str::trim` (Unicode-aware, decodes chars) with a byte scan. The integral casts needed care: my first version of the scan used indexed loops and cost them 20-30% versus the `<[u8]>::trim_ascii` they used before. Rewriting the scan with slice patterns — the same shape std uses for `trim_ascii` — recovers that. There's a comment on `trim_bytes` recording why, so it doesn't get "simplified" back. Also added `cast_string_to_bool_and_float` groups to `cast_from_string.rs`; string → boolean and string → float had no benchmark coverage at all, so the 63% win was invisible to the bench suite. -- 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]
