Copilot commented on code in PR #13282:
URL: https://github.com/apache/gluten/pull/13282#discussion_r4312220585
##########
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:
Hardcoding `UPSTREAM_VELOX_PR_ID` to a specific upstream PR will cause all
builds to pull that PR by default, which can make builds non-reproducible and
break when the PR is unavailable/rewritten. Consider leaving this empty by
default and setting it only via CI/environment (or a separate developer-only
override mechanism) so normal builds consume the pinned Velox dependency.
##########
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 specific exception message substring can be brittle if
upstream wording changes (even when behavior is correct). Prefer asserting on
the error type/code when available (e.g., Velox user error with an overflow
category/code), or match a more stable signal than the full message prefix.
##########
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:
This table cell is very long, which makes the support matrix hard to scan
and churn-prone in future edits. Consider shortening the cell (e.g., a brief
note plus a link/footnote to a dedicated section) or formatting the details as
multiple bullet points below the table.
--
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]
