Repository navigation
Conversation
|
Run Gluten Clickhouse CI on x86 |
## Summary Implement standalone Spark-specific HALF_UP rounding for primitive numeric and DECIMAL inputs. Correct floating decimal midpoints, integral overflow behavior, negative scales, decimal result metadata, and NULL-scale handling. ## Prompting Intent Provide a self-contained ROUND contribution directly on public main, without unrelated numeric-function implementations or feature-branch prerequisites. Preserve Spark 4.1 behavior within the explicit supported scale and JVM domain, and keep unrelated Presto arithmetic unchanged. ## Linked Sources - facebookincubator#10929 - https://github.com/apache/spark/blob/v4.1.1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala - https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Double.html#toString(double) - https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/math/RoundingMode.html#HALF_UP - apache/gluten#13103 ## Rationale Compute HALF_UP rounding directly rather than reusing Presto's binary multiply-and-round behavior. Use wide integer intermediates with explicit LEGACY narrowing and captured ANSI mode, and validate decimal precision before multiplication. Capability-specific integration aliases distinguish the corrected implementation from older registrations and overlays. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1
707345f to
a2d95a6
Compare
|
Run Gluten Clickhouse CI on x86 |
Label: correctness ## Summary Preserve typed NULL scales and validate decimal ROUND through the ordinary native implementation. Enable negative decimal scales within [-400, 400] and retain Spark fallback outside that interval. Add cross-version integration coverage and use the existing upstream-PR developer hook. ## Prompting Intent Address the compatibility review with a focused decimal Gluten companion, using public Spark behavior and maintained native and SQL tests. Keep integral, floating-point and scaled CEIL/FLOOR offload outside this change. ## Linked Sources - facebookincubator/velox#19141 - facebookincubator/velox#19141 (comment) - facebookincubator/velox#19141 (review) - facebookincubator/velox#19469 - apache#13103 ## Rationale NULL is an execution value, not a zero rounding scale. Keep zero only for resolved type calculation and preserve the typed NULL argument. Separate frontend eligibility from the optional general native-validation switch so unsupported scales always retain Spark execution. Use the existing developer hook to validate Velox PR19141 in CI without introducing capability aliases. Replace the temporary hook with a qualified regular dependency before merging. Shared tests compare actual Spark and Gluten results, schemas and execution placement on Spark 3.4 through 4.1, while recognizing the releases' different decimal-overflow diagnostics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1
a2d95a6 to
d66fbf9
Compare
luis4a0
changed the title
There was a problem hiding this comment.
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
3 open findings
UPSTREAM_VELOX_PR_IDhook to a specific PR, which contradicts… · New\"Decimal overflow\") is brittle across… · Newscala.collection.JavaConverters._is deprecated on Scala 2.13+ (used by Spark 4.x). Since this… · New
What changed in this PR
Enables guarded native offload for decimal ROUND with support for NULL/negative scales and adds cross-Spark-version test coverage to validate results, schemas, and fallback behavior.
Changes:
- Preserve typed-NULL
ROUNDscale in Substrait translation and validate decimal scale range[-400, 400](or NULL) for native execution. - Add Spark/Gluten integration tests + Velox validator tests for decimal
ROUNDbehavior across Spark 3.4–4.1. - Update docs to reflect partial support for
roundwith decimal scale guarding and fallback.
| File | Description |
|---|---|
| gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala | Enables the new shared GlutenDecimalRoundSuite for Spark 4.1 Velox backend tests. |
| gluten-ut/spark40/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala | Enables the new shared GlutenDecimalRoundSuite for Spark 4.0 Velox backend tests. |
| gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala | Enables the new shared GlutenDecimalRoundSuite for Spark 3.5 Velox backend tests. |
| gluten-ut/spark34/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala | Enables the new shared GlutenDecimalRoundSuite for Spark 3.4 Velox backend tests. |
| gluten-ut/common/src/test/scala/org/apache/spark/sql/GlutenDecimalRoundSuite.scala | Adds cross-version Spark vs Gluten correctness/schema/native-plan tests for decimal ROUND. |
| gluten-substrait/src/main/scala/org/apache/gluten/expression/DecimalRoundTransformer.scala | Preserves typed NULL scale when translating ROUND to Substrait (instead of serializing as 0). |
| ep/build-velox/src/get-velox.sh | Sets a hardcoded upstream Velox PR ID for builds. |
| docs/velox-backend-scalar-function-support.md | Documents partial support for round with decimal scales in [-400, 400] and NULL. |
| cpp/velox/tests/SparkFunctionTest.cc | Adds native validator test coverage for decimal ROUND scale acceptance/rejection. |
| cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc | Implements native-side validation for decimal ROUND scale constraints and NULL scale. |
| backends-velox/src/test/scala/org/apache/gluten/backendsapi/velox/VeloxDecimalRoundSuite.scala | Adds Scala-side tests asserting typed-NULL serialization and validation behavior even when native validation is disabled. |
| backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala | Adds JVM-side decimal ROUND scale validation to keep guards active when general native validation is disabled. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| # Developer use only for testing Velox PR. | ||
| UPSTREAM_VELOX_PR_ID="" | ||
| UPSTREAM_VELOX_PR_ID="19141" |
There was a problem hiding this comment.
Agreed that this must not become the normal merged dependency. This revision is intentionally a dependency-blocked validation companion: Rui explicitly requested exercising the upstream PR through the existing developer hook at facebookincubator/velox#19141 (comment) . Clearing the hook now would test the stock dependency without the decimal fix. The PR description records the merge gate: land the upstream fix, qualify a normal dependency containing it, then restore the empty hook before merging. Keeping this thread open to track that required dependency step.
There was a problem hiding this comment.
@luis4a0 Yes, this is only for development testing, and we could remove this change after the corresponding Velox gets merged.
|
CI note for the current revision: the ARM The rounding changes do not modify architecture/compiler selection. This |
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
There was a problem hiding this comment.
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
2 open findings
2 resolved since last review
🧠 Review effort: Lite
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
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
There was a problem hiding this comment.
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 dereferenced without verifyingparams.size() >= 1(and more generally, consistency… · NewUPSTREAM_VELOX_PR_IDchanges the default build behavior for everyone… · NewUPSTREAM_VELOX_PR_IDhook to a specific PR, which contradicts…round.scalewhen it is syntactically aLiteral, but the intended… · New
1 resolved since last review
🧠 Review effort: Lite
|
Run Gluten Clickhouse CI on x86 |
rui-mo
left a comment
rui-mo
left a comment
There was a problem hiding this comment.
@luis4a0 Thanks. Please also note that there is a worflow failure.
org.apache.spark.sql.GlutenDecimalRoundSuite *** ABORTED ***
java.lang.RuntimeException: Unable to load a Suite class org.apache.spark.sql.GlutenDecimalRoundSuite that was discovered in the runpath: null
| * validation. | ||
| */ | ||
| override def doExprValidate(substraitExprName: String, expr: Expression): Boolean = expr match { | ||
| case round: Round if round.child.dataType.isInstanceOf[DecimalType] => |
There was a problem hiding this comment.
This validation might be duplicate, as we've added a native validation in the "SubstraitToVeloxPlanValidator.cc" (which is a preferred way with a unified validate API).
There was a problem hiding this comment.
@rui-mo, the bounds overlap, but these checks are not equivalent under the supported validation settings. spark.gluten.sql.enable.native.validation=false skips the JNI/Substrait plan validator. The Spark-expression eligibility hook still runs while constructing the candidate Substrait plan:
- Mandatory expression eligibility:
- Optional native plan validation:
Without the JVM eligibility guard, that configuration could offload an unsupported scale and fail in native execution instead of retaining Spark. For example, Spark 4.1.1 successfully evaluates decimal ROUND at scales 401 and -401, while our documented native domain rejects them.
This uses the existing ValidatorApi.doExprValidate path, not an additional full validation pass. A rejected expression becomes a failed offload candidate through the normal validation/fallback path. I am retaining the guard to preserve this behavior independently of the optional native-validation flag.
GlutenDecimalRoundSuite explicitly exercises this configuration and checks both the fallback plan and Spark-compatible results:
There was a problem hiding this comment.
spark.gluten.sql.enable.native.validation = false
The intention is to perform native validation whenever possible, while avoiding introducing native-engine-specific logic into Scala. Instead, we aim to maintain a unified interface between Scala and native validation.
This configuration is primarily intended for debugging. It forces all computation to be offloaded to the native engine without fallback, meaning that unsupported operations will result in runtime exceptions rather than falling back to the original engine.
Does this approach make sense to you? Feel free to update the relevant documentation if anything is unclear.
There was a problem hiding this comment.
@rui-mo, agreed. I removed the decimal-specific doExprValidate policy from Scala and now rely on the existing native Substrait validator as the normal compatibility gate.
Commit: bb36d43
The behavior is now:
- With native validation enabled (the default), scales outside
[-400, 400]fail native validation and the expression falls back to Spark. - With native validation disabled for debugging, the plan remains native and the registered decimal special form reports its construction-time scale error at execution.
I also updated the internal configuration description to make that debug-mode behavior explicit.
The direct validator regression now verifies that Scala does not filter or eagerly evaluate invalid literals, nonliteral scales, foldable expressions, or an ANSI-true throwing cast. Integration tests cover normal fallback versus debug-mode native failure, while typed NULL and bounded foldable scales continue to execute natively.
Validation passed: native 3/3, API 2/2, Spark 4.1 SQL 4/4 on JDK 17 and 21, full Spark 4.1 reactor build, active Spotless/Scalastyle, license headers, and packaged native-library hash checks. Fresh upstream CI is still required.
## Summary Keep the shared decimal ROUND checks in an abstract common suite and provide concrete suite entry points in the Spark 3.4, 3.5, 4.0 and 4.1 test modules. This prevents ScalaTest discovery from initializing backend settings while running the backend-independent common module. ## Prompting Intent Address the outstanding decimal ROUND review comments after checking which requests were already implemented and answered. Fix the companion's public CI verification blocker without changing rounding semantics or hiding tests. ## Linked Sources - Review request: facebookincubator/velox#19141 (comment) - Earlier compatibility response: facebookincubator/velox#19141 (comment) - Gluten companion: apache#13103 - Spark 4.1 discovery failure: https://github.com/apache/gluten/actions/runs/38745047943/job/116463329992 - Spark 3.5 discovery failure: https://github.com/apache/gluten/actions/runs/38745047943/job/116463330043 ## Rationale The common module has no backend component registration. A concrete suite there is discovered too early and aborts before the version-specific module can run. Abstracting the shared suite preserves every test body, while the small concrete subclasses keep the existing suite names and backend settings. No workflow exclusion, disabled test, duplicated arithmetic or added backend dependency is needed. The original common-module discovery failure was reproduced. The same Maven wildcard discovery now completes in common and executes all four decimal checks in each qualified Spark module, with no ignored tests. Spark 4.1 was also exercised on JDK 21. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1
There was a problem hiding this comment.
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
9 open findings
validateRounddereferencesparams[0]without checkingparams.size(). If the caller ever… · NewUPSTREAM_VELOX_PR_ID=19141is a temporary developer hook and should be… · NewvalidateRounddereferencesparams[0]without checkingparams.size(). If the converted-params…UPSTREAM_VELOX_PR_IDhook to a specific PR, which contradicts…scale.eval(EmptyRow)during validation can throw (e.g., foldable expressions involving… · Newscale.eval(EmptyRow)during validation can throw (e.g., constant-foldable casts that…scala.collection.JavaConverters._is deprecated on Scala 2.13+ in favor of… · New
🧠 Review effort: Lite
| return false; | ||
| } | ||
|
|
||
| const auto& valueType = params[0]->type(); |
|
|
||
| # Developer use only for testing Velox PR. | ||
| UPSTREAM_VELOX_PR_ID="" | ||
| UPSTREAM_VELOX_PR_ID="19141" |
| override def doExprValidate(substraitExprName: String, expr: Expression): Boolean = expr match { | ||
| case round: Round if round.child.dataType.isInstanceOf[DecimalType] => | ||
| val scale = round.scale | ||
| val supported = | ||
| if (scale.dataType != IntegerType || !scale.foldable) { | ||
| false | ||
| } else { | ||
| scale.eval(EmptyRow) match { | ||
| case null => true | ||
| case value: Int => value >= -400 && value <= 400 | ||
| case _ => false | ||
| } | ||
| } |
| import org.apache.spark.sql.internal.SQLConf | ||
| import org.apache.spark.sql.types._ | ||
|
|
||
| import scala.collection.JavaConverters._ |
## Summary Support constant INTEGER scales in the inclusive range [-400, 400] for decimal_round, decimal_ceil and decimal_floor. Reject other non-NULL scales with an explicit user error during special-form construction, independently of ANSI mode. Preserve NULL scales, unary ROUND's default scale and row-local precision-overflow handling. ## Prompting Intent Address the review request to document the decimal scale limitation and reject unsupported native cases. Keep the change focused on the shared decimal special forms and verify the boundaries through registered functions. ## Linked Sources - Review request: facebookincubator#19141 (comment) - Scale-domain discussion: facebookincubator#19141 (comment) - Decimal correctness issue: facebookincubator#19469 - Guarded integration companion: apache/gluten#13103 ## Rationale Enforcing the supported domain in the shared constant-scale extractor keeps ROUND, CEIL and FLOOR consistent and avoids returning unqualified mathematical results for arbitrary INTEGER scales. This is a conservative native API restriction, not an attempt to emulate JVM resource limits or a claim that Spark rejects every scale outside the interval. Construction-time errors are explicit even for zero/NULL input rows and cannot be hidden by TRY. Row-local arithmetic overflow inside the supported interval remains catchable. Checks run once during expression construction, not per row. The new rejection regression failed before the guard was added. Native tests cover both decimal storage widths, both ANSI modes, direct/cast scales, inclusive endpoints, their immediate neighbors and INTEGER extremes. Maintained Gluten tests exercise supported scales and Spark fallback outside the interval on the four qualified Spark versions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1
## Summary Clarify that decimal ROUND's inclusive [-400, 400] interval matches the native supported domain. Gluten retains Spark execution outside it instead of exposing the native unsupported-scale construction error. ## Prompting Intent Keep the integration support documentation consistent with the native decimal scale-validation follow-up requested during review. ## Linked Sources - Native review request: facebookincubator/velox#19141 (comment) - Native decimal change: facebookincubator/velox#19141 - Integration companion: apache#13103 ## Rationale The JVM/native eligibility guards already enforce the same interval, so no integration behavior change is needed. The support table should distinguish Spark fallback from direct native rejection and avoid implying that the interval is only a test-selection convention. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🔵 Needs a closer look
The acknowledged merge gate still requires a qualified normal Velox dependency containing the fix and removal of the temporary PR hook.
9 open findings
UPSTREAM_VELOX_PR_ID=19141is a temporary developer hook and should be…validateRounddereferencesparams[0]without checkingparams.size(). If the caller ever…validateRounddereferencesparams[0]without checkingparams.size(). If the converted-params…UPSTREAM_VELOX_PR_IDhook to a specific PR, which contradicts…scale.eval(EmptyRow)during validation can throw (e.g., foldable expressions involving…scale.eval(EmptyRow)during validation can throw (e.g., constant-foldable casts that…scala.collection.JavaConverters._is deprecated on Scala 2.13+ in favor of…
🧠 Review effort: Balanced
## Summary Merge upstream main c5bd3de into the decimal ROUND companion, preserving its bounded foldable-scale eligibility, typed-NULL handling and native-validation-independent Spark fallback. ## Prompting Intent Refresh the active OSS Gluten branches after addressing the upstream decimal review. Preserve existing behavior and validate against the new public native dependency selected by main. ## Linked Sources - Companion: apache#13103 - Native dependency: facebookincubator/velox#19141 - Main: apache@c5bd3de ## Rationale Keep both histories intact instead of rewriting previously reviewed commits. Retain the relocated support documentation and all existing eligibility checks while adopting main's public native pin. The existing upstream-PR patch hook remains a development validation gate, not a qualified production dependency or a waiver of its pre-merge removal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1
|
Refreshed this branch by merging upstream main c5bd3de, preserving the reviewed history. The bounded foldable-INTEGER/NULL eligibility and Spark fallback when native validation is disabled are unchanged. Rebuilt against main's public Velox dft-2026_10_26 pin with the current upstream decimal patch. The three native registration/validation tests, three JVM eligibility tests, and four Spark 4.1.1 SQL tests pass; the SQL suite passed on JDK 17 and 21. Explicit style checks also pass. The temporary upstream-PR hook remains a development validation gate and is not waived for merge. These are local results for this refresh; fresh CI is still required, and earlier older-Spark results are not claimed as rerun on this merge. Refresh commit: b3f7bb0 |
|
Run Gluten Clickhouse CI on x86 |
## Summary Copy selected decimal values directly when both scale factors equal one, retaining result-type widening and NULL/selection handling. Add shared ROUND/CEIL/FLOOR identity coverage and keep public documentation focused on the supported contract. ## Prompting Intent Address the latest reviewer feedback on the OSS decimal ROUND implementation, preserve Spark-compatible behavior and existing error contracts, and avoid unnecessary arithmetic on identity scales. ## Linked Sources - Review: facebookincubator#19141 (review) - Issue: facebookincubator#19469 - Validation companion: apache/gluten#13103 ## Rationale The shared factor predicate covers identity scales for all three policies. Validated result typing preserves the input scale and never reduces precision here, so a typed copy is sufficient; non-identity arithmetic and overflow handling stay unchanged. Encoding, lazy-input, precision-boundary, selection and result-reuse checks exercise the registered special forms. The related 52-test native suite passes. Documentation retains scale bounds, construction-time errors, and required Spark fallback without review-history rationale. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1
|
I checked the remaining automated review concerns against the current source:
The companion's current CI is green, but the native PR now has a further reviewed identity-copy/documentation revision: Could a maintainer rerun this companion's CI against that updated native head after approving the Velox workflows? The existing green result qualified the previous native revision, not the new one. I cannot rerun upstream workflows with my current repository permissions. |
|
@luis4a0 Thanks for updating! Added one comment at #13103 (comment). |
## Summary Remove the duplicate decimal ROUND eligibility policy from the Velox Scala validator and rely on the existing unified native Substrait validator during normal planning. Document that disabling native validation is a debug mode that may surface unsupported-operation failures at runtime. ## Prompting Intent Address Rui's review concern that backend-specific compatibility logic should not be duplicated in Scala and native code. Preserve normal Spark fallback through native validation while making the disabled-validation behavior explicit and tested. ## Linked Sources - Review discussion: apache#13103 (comment) - Native validator: https://github.com/apache/gluten/blob/b3f7bb0c11abb015c7980bf159ea93485737db2b/cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc - Native implementation: facebookincubator/velox#19141 ## Rationale The native validator already owns decimal scale bounds and typed-NULL support, so the Scala guard duplicated backend policy and eagerly evaluated foldable expressions. In normal mode, native validation rejects unsupported scales and Gluten falls back to Spark. When validation is disabled for debugging, the plan remains native and the registered special form reports its construction-time scale error. Red regressions demonstrated the old Scala guard intercepting invalid literal and foldable scales. Green coverage verifies backend-neutral expression eligibility, an ANSI-true throwing cast that is not eagerly evaluated, normal fallback, debug-mode native failures, typed NULL, foldable expressions, native validation, JDK 17/21, style, licensing, and packaged native libraries. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1
|
Run Gluten Clickhouse CI on x86 |
…patibility-20261014
## Summary Change the temporary upstream-PR dependency hook from GitHub's commit-series `.patch` endpoint to the final `.diff` endpoint for decimal ROUND qualification. ## Prompting Intent Refresh the Gluten decimal companion against current main and the rebased Velox decimal PR while retaining the qualified IBM Velox pin and an explicit temporary dependency gate. ## Linked Sources - Velox decimal PR: facebookincubator/velox#19141 - Gluten companion: apache#13103 - Architecture review: /root/lpenaranda_files/pr-reviews/round-refresh-20261102/round-refresh-review-2-gpt-6-astra.md ## Rationale The rebased Velox PR retains eight logical commits, including historical intermediate integral changes that the later decimal-only split removes. Replaying that commit series cannot apply to IBM `dft-2026_10_30`, while the reviewed final decimal-only delta applies cleanly. The `.diff` endpoint transports that final delta without changing the pin or hiding the merge gate. Both the currently published PR diff and the locally rebased final diff pass exact-pin apply checks. The resulting source matches the native/JVM tree validated by the refreshed native, API, Spark 4.1 JDK 17/21, packaging, style, and license suites. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1
|
Refreshed this branch by merging current Gluten main New head: 577df0a Main advances the normal Velox base to IBM Fresh validation against that prepared dependency passed:
The temporary hook remains a merge gate until a qualified normal Velox pin contains the decimal implementation. |
Implement standalone Spark-specific HALF_UP rounding for primitive numeric and DECIMAL inputs. Correct floating decimal midpoints, integral overflow behavior, negative scales, decimal result metadata, and NULL-scale handling. Provide a self-contained ROUND contribution directly on public main, without unrelated numeric-function implementations or feature-branch prerequisites. Preserve Spark 4.1 behavior within the explicit supported scale and JVM domain, and keep unrelated Presto arithmetic unchanged. - facebookincubator#10929 - https://github.com/apache/spark/blob/v4.1.1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala - https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Double.html#toString(double) - https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/math/RoundingMode.html#HALF_UP - apache/gluten#13103 Compute HALF_UP rounding directly rather than reusing Presto's binary multiply-and-round behavior. Use wide integer intermediates with explicit LEGACY narrowing and captured ANSI mode, and validate decimal precision before multiplication. Capability-specific integration aliases distinguish the corrected implementation from older registrations and overlays. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1
## Summary Support constant INTEGER scales in the inclusive range [-400, 400] for decimal_round, decimal_ceil and decimal_floor. Reject other non-NULL scales with an explicit user error during special-form construction, independently of ANSI mode. Preserve NULL scales, unary ROUND's default scale and row-local precision-overflow handling. ## Prompting Intent Address the review request to document the decimal scale limitation and reject unsupported native cases. Keep the change focused on the shared decimal special forms and verify the boundaries through registered functions. ## Linked Sources - Review request: facebookincubator#19141 (comment) - Scale-domain discussion: facebookincubator#19141 (comment) - Decimal correctness issue: facebookincubator#19469 - Guarded integration companion: apache/gluten#13103 ## Rationale Enforcing the supported domain in the shared constant-scale extractor keeps ROUND, CEIL and FLOOR consistent and avoids returning unqualified mathematical results for arbitrary INTEGER scales. This is a conservative native API restriction, not an attempt to emulate JVM resource limits or a claim that Spark rejects every scale outside the interval. Construction-time errors are explicit even for zero/NULL input rows and cannot be hidden by TRY. Row-local arithmetic overflow inside the supported interval remains catchable. Checks run once during expression construction, not per row. The new rejection regression failed before the guard was added. Native tests cover both decimal storage widths, both ANSI modes, direct/cast scales, inclusive endpoints, their immediate neighbors and INTEGER extremes. Maintained Gluten tests exercise supported scales and Spark fallback outside the interval on the four qualified Spark versions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1
## Summary Copy selected decimal values directly when both scale factors equal one, retaining result-type widening and NULL/selection handling. Add shared ROUND/CEIL/FLOOR identity coverage and keep public documentation focused on the supported contract. ## Prompting Intent Address the latest reviewer feedback on the OSS decimal ROUND implementation, preserve Spark-compatible behavior and existing error contracts, and avoid unnecessary arithmetic on identity scales. ## Linked Sources - Review: facebookincubator#19141 (review) - Issue: facebookincubator#19469 - Validation companion: apache/gluten#13103 ## Rationale The shared factor predicate covers identity scales for all three policies. Validated result typing preserves the input scale and never reduces precision here, so a typed copy is sufficient; non-identity arithmetic and overflow handling stay unchanged. Encoding, lazy-input, precision-boundary, selection and result-reuse checks exercise the registered special forms. The related 52-test native suite passes. Documentation retains scale bounds, construction-time errors, and required Spark fallback without review-history rationale. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d412b830-d346-4c14-b070-3bc5a6cacba1
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🔵 Needs a closer look
Decimal execution relies on unmerged Velox PR #19141 through a temporary development patch hook, requiring human confirmation of a qualified pinned dependency and fresh native/JVM cross-version CI before merge.
9 open issues
UPSTREAM_VELOX_PR_ID=19141is a temporary developer hook and should be…validateRounddereferencesparams[0]without checkingparams.size(). If the caller ever…validateRounddereferencesparams[0]without checkingparams.size(). If the converted-params…UPSTREAM_VELOX_PR_IDhook to a specific PR, which contradicts…scale.eval(EmptyRow)during validation can throw (e.g., foldable expressions involving…scale.eval(EmptyRow)during validation can throw (e.g., constant-foldable casts that…scala.collection.JavaConverters._is deprecated on Scala 2.13+ in favor of…
Review effort: Balanced
What changes are proposed in this pull request?
Focused Gluten companion for facebookincubator/velox#19141,
following the compatibility review at
facebookincubator/velox#19141 (comment).
[-400, 400]or NULL, includingcasts/arithmetic before Spark constant folding. The existing transformer
emits typed literals for standard
decimal_roundexecution.of exposing its construction-time error, including when general native
validation is disabled.
tests. Integral/floating ROUND and scaled CEIL/FLOOR offload are unchanged.
Dependency:
UPSTREAM_VELOX_PR_ID=19141uses the existing developer hookto exercise the upstream change in CI. This is a review/validation companion,
not a request to merge the temporary hook. Replace it with a qualified normal
Velox dependency containing the fix before merging.
How was this patch tested?
VeloxDecimalRoundSuite: typed-NULL serialization and scale guards.SparkFunctionTest.decimalRoundScaleValidation, plus the existing nativescalar-function regressions.
GlutenDecimalRoundSuiteon Spark 3.4.4, 3.5.5, 4.0.2 and 4.1.1: actualSpark versus Gluten results and schemas, native-plan assertions, decimal
storage transitions, 38-digit ties/carry/overflow, NULLs, folded scales and
explicit fallback outside the checked interval. ConstantFolding-disabled
coverage verifies that non-Literal foldable scales still offload safely.
concrete version-specific suites are exercised through normal Maven
wildcard discovery, with all four tests executed and no ignored tests.
Overflow remains an error in both ANSI modes. Spark's overflow diagnostic can
differ across versions/evaluation paths; exact diagnostic-text parity and
arbitrary Int32-scale JVM resource-limit behavior are not claimed.
October 27 refresh
Refreshed this branch by merging upstream main c5bd3de, preserving the reviewed history.
The bounded foldable-INTEGER/NULL eligibility and Spark fallback when native validation is disabled are unchanged. Rebuilt against main's public Velox dft-2026_10_26 pin with the current upstream decimal patch. The three native registration/validation tests, three JVM eligibility tests, and four Spark 4.1.1 SQL tests pass; the SQL suite passed on JDK 17 and 21. Explicit style checks also pass.
The temporary upstream-PR hook remains a development validation gate and is not waived for merge. These are local results for this refresh; fresh CI is still required, and earlier older-Spark results are not claimed as rerun on this merge.
Refresh commit: b3f7bb0
October 30 validation follow-up
Commit: bb36d43
Removed the duplicate decimal-specific Scala eligibility guard in response to:
#13103 (comment)
Normal compatibility policy now lives in the unified native Substrait validator. With native validation enabled, unsupported scales fall back to Spark. Disabling native validation is documented as a debug mode where unsupported native operations may fail at runtime instead of falling back.
Fresh local validation:
The native validator, decimal transformer format, supported
[-400, 400]interval, typed-NULL behavior, and temporary dependency gate are unchanged. Fresh upstream CI and reviewer approval remain required.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.