[GLUTEN-12356][VL] Preserve precision for long decimal to double cast#12605
Draft
Smallfu666 wants to merge 3 commits into
Draft
[GLUTEN-12356][VL] Preserve precision for long decimal to double cast#12605Smallfu666 wants to merge 3 commits into
Smallfu666 wants to merge 3 commits into
Conversation
Pin the rewrite scope with a unit test on genCastWithNewChild (precision <= 18 keeps the direct conversion, > 18 goes through the exact string representation), and add a parquet-backed direct-cast matrix across DECIMAL(18,6)/(19,0)/(38,0)/(38,19)/(38,38) covering positive/negative/zero/NULL values, values around 2^53, and near-max 38-digit values, compared against vanilla Spark under both ANSI on and off. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cast.evalMode does not exist before Spark 3.4 (it replaced the ansiEnabled constructor arg in SPARK-40389), so the 4-arg Cast(child, StringType, timeZoneId, evalMode) form fails to compile on Spark 3.3. Use the 3-arg Cast instead and keep the outer cast via withNewChildren: decimal-to-string is lossless regardless of eval mode, so the original ANSI semantics are preserved by the outer cast. Verified: Spark 3.3 test-compile passes; MathFunctionsValidateSuite (Spark 3.5, Velox) 46/46. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Good catch — Fixed in 2601543 with the 3-arg Cast, keeping the outer cast via Verified: Spark 3.3 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
Fixes #12356.
Velox converts a long decimal's unscaled
int128value todoublebefore applying the decimal scale. For values whose unscaled representation exceeds the precision ofdouble, this can produce a result that differs from Spark, which rounds the exact decimal value once (BigDecimal.doubleValue).This patch keeps the workaround Velox-specific and:
double;genCastWithNewChild: precision <= 18 keeps the direct conversion, precision > 18 goes through the string representation;DECIMAL(18,6),DECIMAL(19,0),DECIMAL(38,0),DECIMAL(38,19),DECIMAL(38,38), covering positive/negative/zero/NULL values, values around 2^53, and near-max 38-digit values, with expected results taken from vanilla Spark, under both ANSI on and off;spark.sql.decimalOperations.allowPrecisionLoss=false);ProjectExecTransformer.Performance
Micro-benchmark: 10M-row parquet
DECIMAL(38,19)column,SELECT SUM(CAST(c AS DOUBLE)),local[8], 3 warmups + 5 measured trials per configuration, medians reported. The pre-patch direct path was emulated in the same binary by temporarily gating the rewrite behind a config (not part of this PR). Each run first executed the issue's query as a correctness probe to prove which path was active: patched and vanilla Spark returned214.4, the direct path returned214.39999999999998.214.4)214.39999999999998)214.4)DECIMAL(18,6)control column is unaffected (within noise), confirming the rewrite only applies to precision > 18.SUMaggregate: floating-point aggregation order differs between engines, so the SUM values agree only to the last ulp (patched Gluten4.999999513013628E13vs vanilla Spark4.999999513013627E13) and are not expected to be bit-identical. Per-row cast equality against the vanilla Spark oracle is covered by the regression tests in this PR.The better long-term fix is a per-row fast path in Velox's
applyDecimalToFloatCast(direct conversion when the unscaled value is exactly representable indouble, exact string otherwise), which would recover most of this cost; this PR keeps a narrow Spark-compatible workaround in Gluten until such a fix lands upstream. If preferred, the rewrite can also be gated behind a config for a staged rollout.How was this patch tested?
mainwith Spark 3.5: vanilla Spark returned214.4, while Gluten/Velox returned214.39999999999998.MathFunctionsValidateSuiteand its ANSI variant with the Velox backend: 46 tests passed, 0 failed (including the 2 new tests above).git diff --check.Was this patch authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex GPT-5.6 Luna; Claude Opus 4.8