vb-dbrks commented on PR #59408: URL: https://github.com/apache/spark/pull/59408#issuecomment-6178418352
Hi @HyukjinKwon - Thanks for suggesting `Float.toString`. I tried it locally... it preserves the familiar Float results you highlighted and passes the existing boundary tests, but I found another compatibility issue. For example, Float `12345.1` actually stores `12345.099609375`. Converting through `Float.toString` makes `floor(x, 1)` return decimal `12345.1`, so Spark evaluates `floor(x, 1) <= x` as false. The current widening approach and a control reproducing the previous `decimal(14,7)` conversion both return `12345.0`. I verified this in interpreted and generated evaluation, with ANSI enabled and disabled. This makes me lean towards retaining widening, beyond its existing Spark precedent. However, I agree that changing ordinary Float results such as `ceil(0.1f, 1)` from `0.1` to `0.2` is a meaningful compatibility decision. The current approach uses the canonical decimal representation after widening; it should not be described as exact binary-value rounding for both Float and Double. I’ve added local tests for the comparison cases and inputs just inside the magnitude limits, including valid rounding carries. The relevant validation passes 251 tests and three style checks with the declared dependencies. These test additions remain unpushed. Would you favour retaining widening with the Float changes documented, or narrowing this PR to the Double defect and addressing Float semantics separately? -- 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]
