akupchinskiy commented on PR #12049: URL: https://github.com/apache/arrow-rs/pull/12049#issuecomment-6610560280
Thanks for the careful review, these are all fair points. Happy to split it up. I'll strip this PR down to just the fix: compute the exact value directly like in your sketch (with `i256` for wider scales), no `ryu`. The two-pass kernel and the other speedups can come later in a separate PR, with the benchmark cases in their own PR first. Fair warning, the fix on its own will be slower in places: high-scale casts of non-integers now always take the exact path (around 3x in my runs on `Decimal128(38, 15)`/`(38, 20)`), and depending on the semantics we pick, the common casts may pay a small per-value check. I'll post numbers for the fix-only version. On semantics, your `1.005` example convinced me: if the definition switches at 2^53, the result depends on the path you cast through, and keeping the old rounding below 2^53 has the same problem for cases like `26.657349498176934`. So I lean towards the exact value everywhere. The one thing that gives me pause is that today's behaviour below 2^53 is fairly common. DuckDB and StarRocks round the `f64` product the same way, so `0.15` at scale 1 gives `0.2` there, and about a third of the ~1% of values that would change match the decimal string people expect. It's not universal: ClickHouse truncates, and SQLite's `round` uses the exact value. Above 2^53, DuckDB, ClickHouse and Polars all behave like arrow does today (`295161.0` at scale 15 gives `295160.999999999967232`). So I'd rather leave the call on below-2^53 behaviour to the maintainers. I have both versions working locally and can go either way. On AI: yes. I found the bug myself while comparing engines on a benchmark. Claude Code helped write the fix, tests and text, and Codex reviewed it. I verified everything myself. I've added a short disclosure to the PR description, and I'll trim the wordier parts. -- 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]
