0lai0 commented on code in PR #5050:
URL: https://github.com/apache/datafusion-comet/pull/5050#discussion_r4288703046
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -441,13 +437,18 @@ object CometCast
case _ => unsupported(DataTypes.DoubleType, toType)
}
- private def canCastFromDecimal(toType: DataType): SupportLevel = toType
match {
- case DataTypes.FloatType | DataTypes.DoubleType | DataTypes.ByteType |
DataTypes.ShortType |
- DataTypes.IntegerType | DataTypes.LongType | DataTypes.BooleanType |
- DataTypes.TimestampType =>
- Compatible()
- case _ => Unsupported(Some(s"Cast from DecimalType to $toType is not
supported"))
- }
+ private def canCastFromDecimal(fromType: DecimalType, toType: DataType):
SupportLevel =
+ toType match {
+ // Negative-scale source overflows natively; see #5013.
+ case DataTypes.ByteType | DataTypes.ShortType | DataTypes.IntegerType |
DataTypes.LongType |
+ DataTypes.TimestampType if fromType.scale < 0 =>
Review Comment:
Good call, thanks. Restructured, though not as an early return.
An early return would not have helped, because the guard only covers a
subset of the target types, so the `Byte | Short | Integer | Long | Timestamp`
list had to appear twice either way. The duplication was the problem, not the
branch position.
It now matches that list once and does the `fromType.scale < 0` check inside
that case, with the comment about why float and double are deliberately
excluded sitting on the case that excludes them. Semantics unchanged,
`CometNativeCastSuite` and `CometExpressionSuite` pass.
##########
spark/src/main/scala/org/apache/comet/serde/arithmetic.scala:
##########
@@ -91,6 +90,25 @@ trait MathBase {
Unsupported(Some(s"Unsupported datatype $dt"))
}
+ // Native decimal Add/Subtract/Divide/IntegralDivide/Remainder scale-aligns
operands by
+ // multiplying by 10^|delta|, which overflows (subtract-with-overflow panic
in debug, silent
+ // wrap in release) whenever any operand or the result has negative scale.
See issue #5013.
+ private[comet] val negScaleDecimalArithmeticReason: String =
+ "Arithmetic on negative-scale decimal is not supported natively"
+
+ private[comet] def negScaleDecimalRejection(expr: BinaryArithmetic):
Option[Unsupported] = {
Review Comment:
You were right @comphead, and most of this file's changes have since come
out. Sorry for the delay.
I had attributed the panic to the operators scale-aligning by `10^|delta|`,
which was wrong. Add and subtract align by `10^(max_scale - s1)`, never
negative. The real cause was two width tests computing `max(s1, s2) as u8 +
max(p1 - s1 as u8, p2 - s2 as u8)`, where a negative scale wraps `s as u8` to
255 and underflows the subtraction. Both are now rewritten in `i16`.
So the guards on `CometAdd`, `CometSubtract` and `CometRemainder` are gone,
along with their `getUnsupportedReasons` overrides. I verified each runs
natively and matches Spark over `Decimal(10,-1)`, `Decimal(20,-5)` and
`Decimal(38,-5)`, and they are pinned in `safe ops on negative-scale decimal
run natively`.
Only `CometDivide` and `CometIntegralDivide` still need it.
`spark_decimal_div` computes its exponents in `u32`, so `s1 = -5` becomes
~4.29e9 and the BigInt branch attempts `10.pow(~4.29e9)`, an unbounded
allocation that hangs rather than fails. `decimal_div` now rejects a negative
scale up front, but there is still no usable native result. The comment carries
that reason now.
--
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]
