Copilot commented on code in PR #12777:
URL: https://github.com/apache/gluten/pull/12777#discussion_r4070561229
##########
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] =>
Review Comment:
RoundCeil and RoundFloor are Spark 4.0 expression classes, while this common
source is compiled for the Spark 3.4/3.5 profiles referenced by the new version
guard in the test. Those profiles will fail compilation because the types are
unavailable; a runtime `assume` cannot prevent that. Move this handling into a
Spark-4-specific shim/source set or use a version-compatible abstraction.
##########
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 assertion accepts any exception, including the
`GlutenNotSupportException` raised by `DecimalCeilFloorTransformer` during
planning, so it does not verify that the expression actually falls back to
Spark or that Spark raises the expected ANSI precision-overflow error. Assert
the specific Spark overflow failure and/or inspect the resulting plan to
confirm the native decimal expression was not used.
--
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]
