Copilot commented on code in PR #13282:
URL: https://github.com/apache/gluten/pull/13282#discussion_r4282071654
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -127,6 +127,15 @@ bool SubstraitToVeloxPlanValidator::validateRound(
return false;
}
+ const auto& scale = arguments[1].value().literal();
+ if (scale.has_null() && scale.null().has_i32()) {
+ const auto& valueType = params[0]->type();
+ if (!valueType->isDecimal() &&
+ (valueType->isTinyint() || valueType->isSmallint() ||
valueType->isInteger() || valueType->isBigint())) {
+ return true;
+ }
+ }
Review Comment:
`params[0]` is dereferenced without checking `params.size()` (or that the
pointer is non-null). If argument conversion ever produces fewer params than
`scalarFunction.arguments()` (or yields a null typed expr), this will read out
of bounds and can crash validation. Add an explicit guard (e.g., require
`params.size() >= 1` and `params[0] != nullptr`) before accessing
`params[0]->type()`, otherwise return false.
##########
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:
Asserting on the substring \"overflow\" is relatively brittle (message
wording can change across Velox versions/localization). If available in this
test framework, prefer asserting on a stable error category/code (or a more
specific message contract) to reduce flaky failures when upstream error text
changes.
##########
ep/build-velox/src/get-velox.sh:
##########
@@ -25,7 +25,7 @@ RUN_SETUP_SCRIPT=ON
ENABLE_ENHANCED_FEATURES=OFF
# Developer use only for testing Velox PR.
-UPSTREAM_VELOX_PR_ID=""
+UPSTREAM_VELOX_PR_ID="19480"
Review Comment:
Hard-coding `UPSTREAM_VELOX_PR_ID` will force all builds to pull/test
against a specific upstream PR, which can silently change behavior and break
reproducibility for non-dev/CI users. Prefer keeping the default empty (or
gated behind an explicit env var/flag) so this hook is only active when
intentionally enabled.
##########
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:
The scale bound `400` is a magic number duplicated across multiple places
(tests/docs/native validation). Consider extracting it into a shared constant
(e.g., `MaxIntegralRoundScale`) to avoid divergence and to make future changes
safer.
--
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]
