wud-if commented on PR #13412: URL: https://github.com/apache/gluten/pull/13412#issuecomment-6542483747
@ggjh-159 Thanks for the review — all four items are addressed and the suite is green locally (29/29, details below). - **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`. I kept the plain list 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. Pinned by `testFloorDecimalResolvesToNumericOnlyConverter` / `testUnaryMinusDecimalResolvesToNegateConverter`, and `testFloorCeilDecimal` is green again end-to-end. - **Typed-null literals crash conversion**: fixed in 720f3c5 — `toVariant` intercepts null literals before the value dispatch and falls back to the null-accepting `VarCharValue(null)` carrier (the same mechanism as the TIME typed-null fix on #13455); velox's `ConstantTypedExpr` renders a null variant as a null constant of the expression type. Pinned by `RexNodeConverterTest.testNumericNullLiteralConvertsToNullCarrier`; all four dual-run cases carrying `CAST(NULL AS ...)` are green. One dependency to disclose: this rides on the velox4j serde that always emits the variant value member for typed-null carriers — merged upstream via #63 (a244d22) — so the CI pin needs to move past that commit for the runtime half (see the last bullet). - **`ROUND(647568.815, 2)` returning 647569**: the offload path was not losing the scale — the rounded 647568.82 is correct inside velox, and the 647569 came from the print channel: the velox print sink formats DOUBLE columns with the C++ ostream default of 6 significant digits (`StringFormatter.h`, `DefaultFormatter<double>` does `ss << t`), so 647568.82 printed as 647569 while the shorter rows (2.68, -2.5) stayed intact — exactly the asymmetry you observed. `testRoundHalfUpTieInDecimalDomain` now asserts through `CAST(ROUND(x, 2) AS STRING)` (c5a1012): the cast renders the double through its shortest round-trip representation, printed verbatim, so the rounded scale survives and the tie keeps its discriminating power. - **Wrong third expectation**: fixed in the same commit — `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]
