Copilot commented on code in PR #12777:
URL: https://github.com/apache/gluten/pull/12777#discussion_r4141110964
##########
gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala:
##########
@@ -390,6 +390,18 @@ object ExpressionConverter extends SQLConfHelper with
Logging {
substraitExprName,
replaceWithExpressionTransformer0(r.child, attributeSeq,
expressionsMap),
r)
+ case rc: RoundCeil if rc.child.dataType.isInstanceOf[DecimalType] =>
+ DecimalCeilFloorTransformer(
+ substraitExprName,
+ replaceWithExpressionTransformer0(rc.child, attributeSeq,
expressionsMap),
+ rc,
Review Comment:
This shared converter is used by the Bolt backend as well as Velox, so these
cases make Bolt emit a two-argument `ceil`/`floor` call for decimal inputs.
Unlike the Velox parser changed in this PR,
`cpp/bolt/substrait/SubstraitParser.cc:263-280` has no arity-sensitive mapping
to `decimal_ceil`/`decimal_floor`, and Bolt's registration only adds the
existing `round` override
(`cpp/bolt/operators/functions/RegistrationAllFunctions.cc:51-58`). This can
make Bolt attempt an unsupported ordinary function (or apply the wrong decimal
semantics); gate this transformer to Velox or add and test the corresponding
Bolt native implementation.
##########
backends-velox/src/test/scala/org/apache/gluten/functions/MathFunctionsValidateSuite.scala:
##########
@@ -122,6 +123,51 @@ class MathFunctionsValidateSuite extends
FunctionsValidateSuite {
}
}
+ test("2-arg ceiling / floor on decimals (RoundCeil / RoundFloor)") {
+ // The 2-argument ceiling/floor SQL forms only exist on Spark 4.0+; on
Spark 3.4/3.5 they are
+ // invalid and would fail during analysis, so skip the test on those
profiles.
+ assume(SparkVersionUtil.gteSpark40)
+ // The 2-arg forms produce Spark RoundCeil / RoundFloor and dispatch to
the Velox
+ // decimal_ceil / decimal_floor special forms. The projection is native
only when the
+ // expression offloads, so checkGlutenPlan[ProjectExecTransformer] doubles
as an offload
+ // assertion; runQueryAndCompare additionally validates results against
vanilla Spark.
+ runQueryAndCompare(
+ "SELECT ceiling(cast(l_quantity as decimal(12, 2)), 1) FROM lineitem
limit 10") {
+ checkGlutenPlan[ProjectExecTransformer]
+ }
+ runQueryAndCompare(
+ "SELECT floor(cast(l_quantity as decimal(12, 2)), 1) FROM lineitem limit
10") {
+ checkGlutenPlan[ProjectExecTransformer]
+ }
+ // Negative scale rounds to the left of the decimal point.
+ runQueryAndCompare(
+ "SELECT ceiling(cast(l_extendedprice as decimal(20, 4)), -2) FROM
lineitem limit 10") {
+ checkGlutenPlan[ProjectExecTransformer]
+ }
+ runQueryAndCompare(
+ "SELECT floor(cast(l_extendedprice as decimal(20, 4)), -2) FROM lineitem
limit 10") {
+ checkGlutenPlan[ProjectExecTransformer]
+ }
+ }
+
+ test("2-arg ceiling / floor on decimals falls back under ANSI overflow") {
+ assume(SparkVersionUtil.gteSpark40)
+ // Velox's decimal_ceil / decimal_floor return NULL when the rounded
result overflows the
+ // declared precision, whereas Spark raises under ANSI mode. Even with
ANSI fallback disabled
+ // (native ANSI execution opted in), this op must fall back to Spark so
the overflow raises
+ // instead of silently producing NULL. DECIMAL(38, 0) at its maximum value
rounded with a
+ // negative scale overflows the 38-digit output precision.
+ withSQLConf(
+ SQLConf.ANSI_ENABLED.key -> "true",
+ GlutenConfig.GLUTEN_ANSI_FALLBACK_ENABLED.key -> "false") {
+ val overflowSql =
+ "SELECT ceiling(cast('99999999999999999999999999999999999999' as
decimal(38, 0)), -1)"
+ intercept[Exception] {
+ spark.sql(overflowSql).collect()
Review Comment:
This test accepts any `Exception`, so it also passes if Gluten fails during
plan conversion (for example, an unhandled `GlutenNotSupportException`) or if
analysis fails before evaluating the overflow. That does not verify the
intended contract that Spark executes the expression and raises its ANSI
overflow error after the project is rejected for native execution; assert the
expected Spark exception/cause and, if possible, inspect the executed plan for
the fallback.
--
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]
