Copilot commented on code in PR #13282:
URL: https://github.com/apache/gluten/pull/13282#discussion_r4283934702
##########
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 accessed without checking `params.size() >= 1`. If param
conversion fails upstream (or the validator is called with an empty `params`
vector despite `arguments` being populated), this will crash. Add an explicit
size check (and ideally also ensure `params[0]` is non-null) before
dereferencing, returning `false` when the converted arguments are not available.
##########
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` to a PR number will cause all builds
(including non-dev/CI jobs) to pull a non-released Velox ref, which can make
builds non-reproducible and unexpectedly break when the PR changes or is
rebased. Prefer leaving it empty by default and setting it via an environment
variable/CI parameter (or gating it behind an explicit `DEVELOPER_MODE` flag).
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala:
##########
@@ -38,12 +39,34 @@ 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 <= MaxQualifiedIntegralRoundScale
+ case _ => false
+ }
+ if (!supported) {
+ logDebug(
+ s"Integral ROUND requires a folded INTEGER scale in [0,
$MaxQualifiedIntegralRoundScale] " +
+ "or NULL; retaining Spark.")
+ }
+ supported
+ case _ => true
+ }
+ case _ => true
+ }
Review Comment:
`doExprValidate` takes `substraitExprName` but the new logic ignores it and
applies solely based on `expr match { case round: Round => ... }`. To keep the
method behavior tightly scoped and avoid accidental coupling if callers ever
pass a `Round` expression under a different `substraitExprName`, consider
guarding the specialized logic with `substraitExprName == \"round\"` (or at
least documenting why matching purely on expression type is the intended
contract).
##########
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), "Arithmetic
overflow: round(");
Review Comment:
Asserting on a hard-coded error-message substring (`\"Arithmetic overflow:
round(\"`) tends to be brittle across Velox version changes and
localization/message refactors. If the test intent is specifically to verify
the *error type/category* (overflow) rather than the exact message text, prefer
asserting on the exception class/code (or a less specific/regex match) so the
test remains stable while still enforcing overflow behavior.
##########
docs/velox-backend-scalar-function-support.md:
##########
@@ -297,7 +297,7 @@ The status applies to `spark.sql.ansi.enabled=false`. When
ANSI mode is enabled,
| randn | Randn | S | |
| random | Rand | S | |
| rint | Rint | S | |
-| round | Round | S | |
+| round | Round | PS | Integral scales in
[0, 400] and NULL use upstream Velox. NULL scales skip child evaluation.
Negative scales, scales outside [0, 400], and scales not folded to INTEGER
literals fall back to Spark to preserve captured ANSI behavior. |
Review Comment:
The support-status column now uses `PS`. If `PS` is a new/valid status, the
document should define it in the status legend/intro section; otherwise readers
won’t know how `PS` differs from existing statuses (e.g., `S`). Please either
add/update the legend to include `PS` (and what it means), or use an existing
documented status code.
--
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]
