wud-if commented on PR #13412: URL: https://github.com/apache/gluten/pull/13412#issuecomment-6543049163
@ggjh-159 Thanks for the thorough local run — all four items are fixed, including the print-layer root cause behind item 3: - **Decimal operands rejected by the numeric guard**: fixed in 78d8d3c — the operand guards now use a decimal-accepting check (`TypeUtils.isNumericOrDecimalType`) instead of the plain `isNumericType`. The plain list stays decimal-free deliberately: the basic arithmetic converters rely on decimals failing that check so the decimal converters (with their promotion casts) take over — widening the shared check would hijack decimal `+ - * /` away from them. The PR's decimal tests (FLOOR/CEIL/ROUND/ABS over DECIMAL) are green again. - **Typed-null literals crash conversion**: fixed in 720f6c5 and, after merging upstream main into this branch (ead4460), unified with the generic `toNullVariant` path that landed there — every `CAST(NULL AS T)` now converts to the null-accepting carrier before the value dispatch (covered by `RexNodeConverterTest`, 4/4 incl. the DECIMAL case); all dual-run cases carrying `CAST(NULL AS ...)` are green. - **`ROUND(647568.815, 2)` returning 647569**: root-caused to the print channel, not the offload computation — the rounded 647568.82 is correct inside velox (as you noted, the round unit test asserts it), and the 647569 came from `DefaultFormatter<double>` rendering through `ss << t`, whose C++ ostream default keeps only 6 significant digits: 647568.82 has 8 and truncated, while the 2.675 row (4 digits) survived — exactly the asymmetry you observed. Fixed at the root in the velox print sink (bigo-sg/velox 1e3854bdf): doubles and floats now render with Java `Double.toString` / `Float.toString` semantics (`std::to_chars` shortest round-trip, plain notation within [1e-3, 1e7), Java-style scientific notation outside — pinned by `testDoubleFormatterToString` with the 647568.82 regression case). The test here now asserts the raw double output (`select ROUND(x, 2)`, no CAST workaround) so it pins the actual offload output path end to end, and the CI pin was switched to the velox4j branch carrying that velox head (pin-velox-arithmetic @ 8cde150 → velox 1e3854bdf, ca0bf2f) so CI exercises the fixed stack. - **Wrong third expectation**: fixed in c5a1012 — `ROUND(-2.5, 2)` keeps -2.5 (no tie at scale 2), matching Flink. -- 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]
