luis4a0 commented on code in PR #13282:
URL: https://github.com/apache/gluten/pull/13282#discussion_r4283757217
##########
cpp/velox/tests/SparkFunctionTest.cc:
##########
@@ -125,6 +126,54 @@ TEST_F(SparkFunctionTest, roundWithDecimal) {
runRoundWithDecimalTest<int8_t>(testRoundWithDecIntegralData<int8_t>());
}
+TEST_F(SparkFunctionTest, integralRoundUsesSessionAnsi) {
+ const auto input = makeRowVector({makeNullableFlatVector<int8_t>({127, -128,
25, -25, std::nullopt})});
+ for (const auto ansi : {false, true}) {
+ queryCtx_->testingOverrideConfigUnsafe({{sparkAnsiEnabledConfigKey(), ansi
? "true" : "false"}});
+ const auto expression = "round(c0, cast(-1 as integer))";
+ if (ansi) {
+ VELOX_ASSERT_USER_THROW(evaluate(expression, input), "overflow");
Review Comment:
The existing VELOX_ASSERT_USER_THROW already checks the VeloxUserError
category. I additionally tightened the message assertion to the upstream
ROUND-specific contract, `Arithmetic overflow: round(`, instead of generic
`overflow`, so an unrelated arithmetic failure cannot satisfy it. Commit:
https://github.com/apache/gluten/commit/a85c8b7f5b25291f6ea8a8e9e1506c2cdb74142e
.
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala:
##########
@@ -38,12 +39,32 @@ import io.substrait.proto.SimpleExtensionDeclaration
import scala.collection.JavaConverters._
import scala.collection.mutable.ArrayBuffer
-class VeloxValidatorApi extends ValidatorApi {
+class VeloxValidatorApi extends ValidatorApi with Logging {
import VeloxValidatorApi._
- /** For velox backend, key validation is on native side. */
- override def doExprValidate(substraitExprName: String, expr: Expression):
Boolean =
- true
+ /**
+ * Enforce integral ROUND eligibility on the JVM to preserve Spark's
captured ANSI semantics,
+ * independently of optional native plan validation.
+ */
+ override def doExprValidate(substraitExprName: String, expr: Expression):
Boolean = expr match {
+ case round: Round =>
+ round.child.dataType match {
+ case ByteType | ShortType | IntegerType | LongType =>
+ // Negative scales can overflow, and Spark captures ANSI mode in the
expression.
+ val supported = round.scale match {
+ case Literal(null, IntegerType) => true
+ case Literal(scale: Int, IntegerType) => scale >= 0 && scale <= 400
+ case _ => false
+ }
Review Comment:
Named the production policy boundary `MaxQualifiedIntegralRoundScale` and
reused it in the guard and diagnostic:
https://github.com/apache/gluten/commit/a85c8b7f5b25291f6ea8a8e9e1506c2cdb74142e
. The comment distinguishes this qualified compatibility bound from the native
INTEGER scale limit. Tests and documentation intentionally retain independent
literal boundary expectations, so changing the production constant cannot
silently widen the tested contract.
--
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]
