[VL] Use upstream integral ROUND and preserve safe SQL fallback by luis4a0 · Pull Request #13282 · apache/gluten

luis4a0 · 2026-10-14T09:11:07Z

What changes are proposed in this pull request?

Separate Gluten companion for
facebookincubator/velox#19480.

Remove only the integral ROUND overlay registrations so they no longer
shadow the upstream implementation. Floating-point overlays are unchanged.
Test the actual registered native implementation's HALF_UP ties, session
ANSI overflow, legacy wrapping, TRY and NULL behavior.
Keep nonnegative integral scales in [0, 400] eligible for native execution.
Typed NULL scales execute natively and skip child evaluation, including
children that would otherwise overflow.
Negative and unqualified scales remain in Spark, even when general native
validation is disabled.

Negative-scale SQL offload is intentionally not enabled here. Spark
captures ANSI mode in the expression, while a native session may have a
different mode later. The fallback preserves that contract, including session
changes before and after physical planning. No new function alias or per-call
BOOLEAN argument is introduced.

Dependency: UPSTREAM_VELOX_PR_ID=19480 uses the existing developer hook
for CI validation. Replace it with a qualified normal Velox dependency
containing the upstream implementation before merging.

How was this patch tested?

A native regression reproduced the old overlay returning 20 for
round(25, -1) and failing to raise ANSI overflow.
The updated native scalar-function suite verifies the upstream
registrations, including 30 for the same HALF_UP tie.
VeloxIntegralRoundSuite checks guards independently of the general
native-validation switch.
GlutenIntegralRoundSuite checks all signed widths, native identity
execution, negative-scale Spark fallback, ANSI overflow, legacy wrapping,
captured-mode changes, NULL-scale child short-circuiting and extreme-scale
fallback on Spark 4.1.1 with
JDK 17 and JDK 21.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: GitHub Copilot CLI 1.0.91

Co-authored by Luis Penaranda and GitHub Copilot CLI 1.0.91.

Label: correctness ## Summary Remove integral ROUND overlay registrations that shadow the upstream implementation, leaving the floating-point overlay unchanged. Validate typed NULL scales natively and preserve child short-circuiting. Retain Spark SQL execution for negative and unqualified integral scales. ## Prompting Intent Provide a separate Gluten companion for the upstream integral ROUND fix using ordinary function names and session ANSI configuration. Exercise the actual native registrations and preserve Spark's captured-mode contract without adding aliases or per-call BOOLEAN arguments. ## Linked Sources - facebookincubator/velox#19480 - facebookincubator/velox#19480 (comment) - facebookincubator/velox#19480 (comment) - facebookincubator/velox#19141 (review) ## Rationale The old overlay returns 20 for the HALF_UP tie round(25, -1) and does not enforce the upstream session ANSI overflow contract. Reusing the upstream registrations avoids maintaining another arithmetic implementation. Positive scales are identity; typed NULL scales are verified to skip even a throwing child. Negative SQL scales remain in Spark because expressions capture ANSI mode while a native session can change later. Guards remain active with general native validation disabled. Use the existing developer hook for Velox PR19480 during CI validation. Replace the temporary hook with a qualified regular dependency before merging. Native, API and Spark 4.1 integration tests cover registration, boundaries, NULLs, overflow, wrapping and captured-mode fallback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

5 open findings

Hardcoding UPSTREAM_VELOX_PR_ID changes the default build behavior to always pull a specific… · New
The debug message is misleading: supported is also false for out-of-range positive literal scales… · New
Validation is now converting arguments[0] into a Velox expression just to determine its type.… · New
The phrase 'Negative and unqualified extreme integral scales' is ambiguous (it reads like only… · New
This comment suggests integral ROUND broadly 'uses Velox', but integral execution is still… · New

What changed in this PR

Aligns integral ROUND behavior with upstream Velox by removing Gluten’s integral overlay registrations while preserving Spark fallback semantics where Spark/Veloх contracts differ (e.g., negative scales, captured ANSI overflow).

Changes:

Removes integral ROUND overlay registrations, keeping only floating-point overlays.
Adds Scala and C++ tests covering integral ROUND native execution, ANSI/legacy overflow behavior, NULL-scale short-circuiting, and fallback rules.
Extends plan validation to allow typed-NULL integral scales natively and updates documentation/status.

File	Description
gluten-ut/spark41/src/test/scala/org/apache/spark/sql/GlutenIntegralRoundSuite.scala	Adds Spark 4.1 Gluten integration tests for integral `ROUND` native vs Spark fallback behavior.
gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala	Enables the new Gluten integral-round suite in Velox test settings.
ep/build-velox/src/get-velox.sh	Sets a default upstream Velox PR hook ID for CI/dev validation.
docs/velox-backend-scalar-function-support.md	Updates `round` support status/details to reflect new integral behavior and constraints.
cpp/velox/tests/SparkFunctionTest.cc	Adds native (Velox) unit tests for integral `ROUND` ANSI behavior and NULL-scale validation.
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc	Adjusts round validation to accept typed-NULL i32 scales for integral inputs.
cpp/velox/operators/functions/overlay/RegisterFunctionOverlay.cc	Removes integral `ROUND` overlay registration; keeps floating-point overlay only.
backends-velox/src/test/scala/org/apache/gluten/backendsapi/velox/VeloxIntegralRoundSuite.scala	Adds validator-focused Scala tests for scale gating independent of global native validation.
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala	Implements integral `ROUND` scale gating (literal [0,400] or typed NULL) to preserve Spark fallback semantics.

🧠 Review effort: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

luis4a0 · 2026-10-14T10:13:24Z


 # Developer use only for testing Velox PR.
-UPSTREAM_VELOX_PR_ID=""
+UPSTREAM_VELOX_PR_ID="19480"


Agreed for the merge-ready revision. This is an explicitly dependency-blocked validation companion, and the hook currently makes CI exercise facebookincubator/velox#19480 rather than a stock dependency without those integral registrations. The PR description requires replacing it with a qualified normal dependency and restoring the empty hook before merging. An environment-only opt-in without updating CI would stop validating the required implementation. Keeping this thread open as the dependency/merge gate; the temporary setting is not proposed as a permanent default.

Label: review-hardening ## Summary Reuse already-converted arguments when validating integral ROUND. Add a computed-input regression and clarify the fallback diagnostic, support documentation and overlay-registration comment. ## Prompting Intent Address review feedback while preserving numerical behavior, existing eligibility guards and support for computed expressions. ## Linked Sources - apache#13282 (comment) - apache#13282 (comment) - apache#13282 (comment) - apache#13282 (comment) ## Rationale Scalar-function validation already produces typed arguments, so ROUND can inspect their result types without another conversion or a field-only restriction. The regression uses a VARCHAR child cast to TINYINT to distinguish the computed type from the underlying field type. The diagnostic and documentation now explicitly cover negative, out-of-range and non-folded scales. Registration is described separately from SQL eligibility. The supported domains, dependency hook, and arithmetic are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1

Label: review-hardening ## Summary Reuse typed arguments already prepared by scalar-function validation instead of converting the decimal ROUND input a second time. Add regression coverage for computed decimal inputs, including negative and NULL scales. ## Prompting Intent Address the review's redundant-conversion concern consistently across the ROUND companions without restricting valid computed child expressions. ## Linked Sources - apache#13282 (comment) - apache#13103 ## Rationale The scalar-function validator has already validated and converted every argument before dispatching to the ROUND helper. Passing that existing typed-expression vector follows the neighboring extract validator pattern and preserves conversion/error handling while removing redundant work. Checking the computed expression type retains support for casts rather than restricting eligibility to direct field references. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

4 open findings

params[0] is accessed without verifying params is non-empty (and that params[0] is non-null).… · New
Hardcoding UPSTREAM_VELOX_PR_ID changes the default build behavior to always pull a specific…
Hard-coding UPSTREAM_VELOX_PR_ID to a specific PR will cause all builds that source this script… · New
The scaladoc on doExprValidate now contradicts the implementation: key validation is no longer… · New

4 resolved since last review

🧠 Review effort: Lite

Label: documentation ## Summary Correct the JVM eligibility ScalaDoc and document the native validator's aligned, non-null argument precondition. No executable behavior changes. ## Prompting Intent Address the JVM/native validation documentation comment and verify the reported params[0] risk against the actual caller and conversion paths. ## Linked Sources - apache#13282 (comment) - apache#13282 (comment) ## Rationale Integral ROUND guards run on the JVM independently of optional native plan validation to preserve Spark's captured ANSI semantics. The private native helper's sole caller creates one non-null typed expression per argument and does not continue after a failed conversion. Its existing arity guard therefore protects the index. Documenting the verified contract avoids redundant checks and does not conceal conversion errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1

Label: documentation ## Summary Document independent JVM-side decimal ROUND eligibility and the aligned, non-null typed-argument contract of the native ROUND validator. ## Prompting Intent Address review comments consistently across the ROUND companions without changing executable behavior or adding redundant fallback paths. ## Linked Sources - apache#13282 (comment) - apache#13282 (comment) - apache#13103 ## Rationale The JVM guard is intentionally independent of optional native validation. The private native helper receives one successfully converted expression per Substrait argument; its caller exits or throws before dispatch on failure. Document those responsibilities instead of suggesting native-only validation or adding a silent fallback for an unreachable parameter state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

5 open findings

params[0] is dereferenced without checking params.size() (or that the pointer is non-null). If… · New
Hardcoding UPSTREAM_VELOX_PR_ID changes the default build behavior to always pull a specific…
Asserting on the substring \"overflow\" is relatively brittle (message wording can change across… · New
Hard-coding UPSTREAM_VELOX_PR_ID will force all builds to pull/test against a specific upstream… · New
The scale bound 400 is a magic number duplicated across multiple places (tests/docs/native… · New

3 resolved since last review

🧠 Review effort: Lite

Label: review-hardening ## Summary Name the qualified integral ROUND scale limit and reuse it in the diagnostic. Strengthen the native regression to match the ROUND-specific arithmetic overflow message while retaining the VeloxUserError category assertion. ## Prompting Intent Address review suggestions about the qualification boundary and overflow assertion without changing integral eligibility or arithmetic. ## Linked Sources - apache#13282 (comment) - apache#13282 (comment) ## Rationale The value 400 is a qualification-policy boundary, not the native scale limit. Give the production guard a descriptive constant while leaving independent literal expectations in tests and documentation to detect unintended drift. The existing assertion already checks the user-error type; the more specific message prevents an unrelated arithmetic failure from satisfying the test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

6 open findings

params[0] is accessed without checking params.size() >= 1. If param conversion fails upstream… · New
Hard-coding UPSTREAM_VELOX_PR_ID to a PR number will cause all builds (including non-dev/CI jobs)… · New
Hardcoding UPSTREAM_VELOX_PR_ID changes the default build behavior to always pull a specific…
doExprValidate takes substraitExprName but the new logic ignores it and applies solely based on… · New
Asserting on a hard-coded error-message substring (\"Arithmetic overflow: round(\") tends to be… · New
The support-status column now uses PS. If PS is a new/valid status, the document should define… · New

4 resolved since last review

🧠 Review effort: Lite

+  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;
+    }
+  }



 # Developer use only for testing Velox PR.
-UPSTREAM_VELOX_PR_ID=""
+UPSTREAM_VELOX_PR_ID="19480"


+  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
+  }


+    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(");


 | 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. |


Copilot AI balanced review requested due to automatic review settings October 14, 2026 09:11

luis4a0 mentioned this pull request Oct 14, 2026

fix(sparksql): Implement and register integral ROUND with ANSI handling facebookincubator/velox#19480

Open

Copilot AI reviewed Oct 14, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 14, 2026 10:39

Copilot AI reviewed Oct 14, 2026

View reviewed changes

Comment thread cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc

Comment thread ep/build-velox/src/get-velox.sh

Comment thread backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala Outdated

github-actions Bot added CORE works for Gluten Core BUILD VELOX DOCS labels Oct 14, 2026

Copilot stopped reviewing on behalf of luis4a0 due to an error October 14, 2026 11:30
The job was not acquired by a runner even after multiple attempts.

Copilot stopped reviewing on behalf of luis4a0 due to an error October 14, 2026 11:54
The job was not acquired by a runner even after multiple attempts.

Copilot AI balanced review requested due to automatic review settings October 15, 2026 10:35

Copilot AI reviewed Oct 15, 2026

View reviewed changes

Comment thread cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc

Comment thread cpp/velox/tests/SparkFunctionTest.cc Outdated

Comment thread ep/build-velox/src/get-velox.sh

Comment thread backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala

Copilot stopped reviewing on behalf of luis4a0 due to an error October 15, 2026 11:12
Workflow completed but conclusion is not available. This may indicate rate limiting or API issues.

Copilot AI balanced review requested due to automatic review settings October 15, 2026 13:42

Copilot AI reviewed Oct 15, 2026

View reviewed changes

Copilot stopped reviewing on behalf of luis4a0 due to an error October 15, 2026 14:30
The job was not acquired by a runner even after multiple attempts.

Navigation Menu

Repository navigation

[VL] Use upstream integral ROUND and preserve safe SQL fallback - #13282

[VL] Use upstream integral ROUND and preserve safe SQL fallback#13282
luis4a0 wants to merge 4 commits into
apache:mainfrom
luis4a0:feat/integral-round-20261014

luis4a0 commented Oct 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

luis4a0 Oct 14, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

luis4a0 commented Oct 14, 2026

What changes are proposed in this pull request?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

luis4a0 Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants