DanielLeens commented on PR #12920: URL: https://github.com/apache/seatunnel/pull/12920#issuecomment-6383285863
Thanks @zhengxiang378928908-code for the careful comparison. Sorry for missing this in the last review: I accepted the "like `CAST` in MySQL, PostgreSQL and H2" wording without checking it against FLOAT/DOUBLE sources near a rounding boundary. This is a carryover from my earlier round, not something new on this head. I re-checked it on `7c4b22ea19fa` and you are right. In `SystemFunction#castAs`, a `Float`/`Double` source still goes through `new BigDecimal((Double) v1)` (the exact binary value, kept from #11523), and only the rounding mode is now unified to `HALF_UP`. For `1.15d` the exact value is `1.149999999999999911...`, so scale 1 gives `1.1`, while the same spelling as STRING/DECIMAL gives `1.2`. So the code behavior is fine and intended, but three places currently claim more than it does: - `docs/en` and `docs/zh` `incompatible-changes.md`: "Every source type now rounds half away from zero, like `CAST` in MySQL, PostgreSQL and H2." - The new comment in `SystemFunction.java` above `setScale(scale, RoundingMode.HALF_UP)`. - The test comments in `SystemFunctionTest` saying the same value must produce the same DECIMAL whatever the source type (true for the exactly representable `1.25`, not in general). @fatmanverse, a minimal fix that keeps the code unchanged: 1. Reword the compatibility note (en/zh) to say that every source type now uses half-away-from-zero rounding, that FLOAT and DOUBLE are still converted from their exact binary value first, and that they can therefore differ from STRING/DECIMAL and from database casts near a rounding boundary (for example `CAST(1.15d AS DECIMAL(10,1))` gives `1.1`). Drop or narrow the database-equivalence claim accordingly. 2. Narrow the two code/test comments the same way. 3. Add one test case such as `1.15d` at scale 1 expecting `1.1` (next to the STRING/DECIMAL `1.15` -> `1.2`), so the documented difference is pinned. Changing the DOUBLE conversion itself (for example to `BigDecimal.valueOf`) would be a separate compatibility decision and should stay out of this PR, as @zhengxiang378928908-code noted. Separately, the branch is now in conflict with `dev` (ahead 3 / behind 29). Please sync with the latest `dev` and resolve the conflict when you push the wording update; the earlier shared unit-test failure is already fixed on `dev` via #12938. I will re-review the new head once it is up. -- 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]
