fix: fall back to Spark for negative-scale decimal casts and arithmetic - #5050
Closed
0lai0 wants to merge 2 commits into
Closed
fix: fall back to Spark for negative-scale decimal casts and arithmetic#50500lai0 wants to merge 2 commits into
0lai0 wants to merge 2 commits into
Conversation
0lai0
force-pushed
the
fix-5013-negative-scale-decimal-cast
branch
from
July 27, 2026 14:35
f4a027d to
e767210
Compare
Member
|
This may overlap with #4799 |
Contributor
Author
|
Thanks @andygrove and @comphead, sorry for missing #4799! That would be great if it covers it. |
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.
Which issue does this PR close?
Closes #5013
Rationale for this change
With
spark.sql.legacy.allowNegativeScaleOfDecimal=true, Spark allowsDecimalType(p, s)withs < 0. Comet's native cast and decimal arithmetic kernels scale-align values by computing10^|scale|(e.g.10_i128.pow(scale as u32)). Whenscaleis negative, casting it tou32produces a huge value, so the power computation overflows: it panics (attempt to multiply/subtract with overflow) in debug builds and silently wraps to an incorrect value in release builds (the native workspace setsoverflow-checks = falsefor release).This is reachable from two places:
Byte/Short/Integer/Long) to/from a negative-scaleDecimalType, or casting a negative-scaleDecimalTypetoTimestampType, both hit the same10^|scale|computation innumeric.rs/cast_decimal_to_timestamp.Add/Subtract/Divide/IntegralDivide/Remainder), which scale-aligns operands the same way.CometCastalready special-cased negative-scale decimals for theDecimal -> Stringdirection, but that guard was unreachable in practice because producing the negative-scale value in the first place would panic before a-> Stringcast could ever run.Since
allowNegativeScaleOfDecimalis a rarely-used legacy flag (defaultfalse), and the native kernels don't implement negative-scale-safe rescaling, the fix makes Comet detect these cases at planning time and fall back to Spark instead of attempting them natively.What changes are included in this PR?
CometCast.scala: addednegativeScaleDecimalCastReasonand marked asUnsupported:int -> Decimal(neg)viacanCastFromByte/Short/Int/LongDecimal(neg) -> intandDecimal(neg) -> TimestampviacanCastFromDecimal(now takes the sourceDecimalTypeso it can checkfromType.scale)All other cast directions to/from negative-scale decimals (e.g.
String <-> Decimal(neg),Decimal(neg) -> Float/Double/String/Boolean/wider-Decimal) don't perform integer scale-alignment and are unaffected, they keep running natively.arithmetic.scala: addednegScaleDecimalRejection(expr: BinaryArithmetic)inMathBase, returningUnsupportedwhen the left operand, right operand, or result type is a negative-scaleDecimalType. Wired intoCometAdd,CometSubtract,CometDivide,CometIntegralDivide,CometRemainder.CometMultiplyandUnaryMinusare intentionally left unguarded, neither scale-aligns, so they stay nativeHow are these changes tested?
Added tests to
CometCastSuiteandCometExpressionSuitemake core./mvnw -pl spark test -Dsuites='org.apache.comet.CometCastSuite'./mvnw -pl spark test -Dsuites='org.apache.comet.CometExpressionSuite'Note: the
TimestampTypecase is not called out in the linked issue's reproducer, it was found while auditing the same10^|scale|code path during this fix and is included here since it shares the exact same root cause and fix.