minni31 commented on code in PR #12777: URL: https://github.com/apache/gluten/pull/12777#discussion_r4141112701
########## gluten-substrait/src/main/scala/org/apache/gluten/expression/DecimalCeilFloorTransformer.scala: ########## @@ -0,0 +1,63 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.gluten.expression + +import org.apache.gluten.backendsapi.BackendsApiManager +import org.apache.gluten.exception.GlutenNotSupportException + +import org.apache.spark.sql.catalyst.expressions._ +import org.apache.spark.sql.types.{DataType, DecimalType} + +/** + * Transformer for Spark `RoundCeil(decimal, scale)` and `RoundFloor(decimal, scale)`. These power + * the 2-argument forms of `ceiling(x, scale)` / `floor(x, scale)` and dispatch to the Velox + * `decimal_ceil` / `decimal_floor` special forms (substrait names `ceil` / `floor`, remapped on the + * C++ side based on arity + decimal arg type). + * + * The output `DataType` is recomputed from the original Spark decimal input type and the constant + * folded scale, matching Spark's `RoundBase.dataType` formula. Mirrors the structure of + * `DecimalRoundTransformer`. + */ +case class DecimalCeilFloorTransformer( + substraitExprName: String, + child: ExpressionTransformer, + original: Expression, + scaleExpr: Expression) + extends BinaryExpressionTransformer { + + private val toScale: Int = { + val evaluated = scaleExpr.eval(EmptyRow) + if (evaluated == null) { + throw new GlutenNotSupportException( + s"Scale expression evaluated to null for ${original.nodeName}. Falling back to Spark.") + } + evaluated.asInstanceOf[Int] + } Review Comment: Fixed in 7c1830872. The transformer now requires a foldable scale and converts evaluation failures, null results, and unexpected result types into `GlutenNotSupportException`, so unsupported scale expressions fall back cleanly instead of aborting transformation. ########## backends-velox/src/test/scala/org/apache/gluten/functions/MathFunctionsValidateSuite.scala: ########## @@ -122,6 +122,30 @@ class MathFunctionsValidateSuite extends FunctionsValidateSuite { } } + test("2-arg ceiling / floor on decimals (RoundCeil / RoundFloor)") { + // 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. Review Comment: Rechecked this against the supported Spark binaries while addressing the review. Spark 3.4/3.5 already register the two-argument `ceil`/`ceiling`/`floor` builders and construct `RoundCeil`/`RoundFloor` (the overload predates Spark 4.0). In 7c1830872, I removed the incorrect Spark-4-only gate so these tests now run across all supported profiles. ########## cpp/velox/substrait/SubstraitParser.cc: ########## @@ -287,6 +287,13 @@ std::string SubstraitParser::mapToVeloxFunction(const std::string& substraitFunc if (substraitFunction == "round") { return "decimal_round"; } + // Spark RoundCeil / RoundFloor are emitted with substrait names "ceil" + // and "floor" but require dispatch to the 2-arg decimal special forms. + // The unary forms `ceil(decimal)` / `floor(decimal)` keep their original + // name (handled by simple-function registration). + if (numArgs == 2 && (substraitFunction == "ceil" || substraitFunction == "floor")) { + return "decimal_" + substraitFunction; Review Comment: Fixed in 7c1830872. Overflow-prone decimal directional rounding now falls back to Spark whenever rounding can exceed the capped precision, regardless of ANSI mode. The regression coverage checks both `ceiling` and `floor`, ANSI on and off, the scale -38 threshold, and the `Int.MinValue` scale boundary. ########## 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: Verified while fixing the review: `RoundCeil` and `RoundFloor` are present in the Spark 3.4 and 3.5 Catalyst APIs and are already exercised by the version-specific Spark expression suites. The common converter therefore remains version-compatible; 7c1830872 also runs the SQL coverage on all supported Spark profiles instead of gating it to Spark 4.x. ########## 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: Fixed in 7c1830872. The overflow tests now inspect the executed plan and require `ProjectExecTransformer` to be absent, then assert that the thrown exception is an arithmetic overflow (directly or through its cause chain). This distinguishes genuine Spark fallback/overflow behavior from an arbitrary planning exception. ########## 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) Review Comment: Fixed in 7c1830872. The normal native-offload comparisons now run explicitly with ANSI disabled. Separate coverage enables ANSI with `spark.gluten.sql.ansiFallback.enabled=false` and verifies that precision-safe decimal ceiling/floor expressions still offload natively. ########## gluten-substrait/src/main/scala/org/apache/gluten/expression/DecimalCeilFloorTransformer.scala: ########## @@ -0,0 +1,96 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.gluten.expression + +import org.apache.gluten.backendsapi.BackendsApiManager +import org.apache.gluten.exception.GlutenNotSupportException + +import org.apache.spark.sql.catalyst.expressions._ +import org.apache.spark.sql.internal.SQLConf +import org.apache.spark.sql.types.{DataType, DecimalType} + +/** + * Transformer for Spark `RoundCeil(decimal, scale)` and `RoundFloor(decimal, scale)`. These power + * the 2-argument forms of `ceiling(x, scale)` / `floor(x, scale)` and dispatch to the Velox + * `decimal_ceil` / `decimal_floor` special forms (substrait names `ceil` / `floor`, remapped on the + * C++ side based on arity + decimal arg type). + * + * The output `DataType` is recomputed from the original Spark decimal input type and the constant + * folded scale, matching Spark's `RoundBase.dataType` formula. Mirrors the structure of + * `DecimalRoundTransformer`. + */ +case class DecimalCeilFloorTransformer( + substraitExprName: String, + child: ExpressionTransformer, + original: Expression, + scaleExpr: Expression) + extends BinaryExpressionTransformer { + + // Velox's `decimal_ceil` / `decimal_floor` return NULL when the rounded result exceeds the + // declared decimal precision, whereas Spark's `RoundBase` raises a precision-overflow error + // under ANSI mode (e.g. DECIMAL(38, 0) at its maximum value rounded with a negative scale). + // Offloading under ANSI would silently substitute NULL for that error, so fall back to vanilla + // Spark and preserve the ANSI semantics. Under non-ANSI mode Spark also returns NULL on + // overflow, matching Velox, so offloading is safe. + if (SQLConf.get.ansiEnabled) { + throw new GlutenNotSupportException( + s"${original.nodeName} on decimal is not offloaded under ANSI mode because Velox returns " + + "NULL on precision overflow while Spark raises. Falling back to Spark.") Review Comment: Fixed in 7c1830872. The blanket ANSI rejection was replaced with a selective type-level guard: fallback occurs only when rounding can change the value and the uncapped output precision exceeds Spark's maximum. Safe ANSI cases, including no-op `DECIMAL(38, s)` cases, remain eligible for native execution. Unsupported negative-input-scale decimals also fall back explicitly because Velox cannot represent them safely. -- 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]
