andygrove opened a new issue, #7021: URL: https://github.com/apache/datafusion-comet/issues/7021
### What is the problem the feature request solves? Follow-up from #5050. Once it lands, ten serde checks recognize a negative-scale decimal, which only `spark.sql.legacy.allowNegativeScaleOfDecimal=true` allows. Each is written inline as `case d: DecimalType if d.scale < 0`, and between them they use five reason strings: | Check | Expressions | Reason | | --- | --- | --- | | [`CometCast.canCastToString`](https://github.com/apache/datafusion-comet/blob/29c4509a5b609ddf7d0706a69d213f6339bad161/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala#L359-L369) | decimal to string | `negativeScaleDecimalToStringReason` | | [`CometCast.canCastFromByte`, `Short`, `Int`, `Long`](https://github.com/apache/datafusion-comet/blob/29c4509a5b609ddf7d0706a69d213f6339bad161/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala#L429-L430) | integer to decimal | `negativeScaleDecimalCastReason` | | [`CometCast.canCastFromDecimal`](https://github.com/apache/datafusion-comet/blob/29c4509a5b609ddf7d0706a69d213f6339bad161/spark/src/main/scala/org/apache/comet/expressions/CometCast.scala#L518-L522) | decimal to integer or timestamp | `negativeScaleDecimalCastReason` | | [`MathBase.negScaleDecimalRejection`](https://github.com/apache/datafusion-comet/blob/29c4509a5b609ddf7d0706a69d213f6339bad161/spark/src/main/scala/org/apache/comet/serde/arithmetic.scala#L105-L119) | `/`, `div` | `negScaleDecimalArithmeticReason` | | [`CometRound`](https://github.com/apache/datafusion-comet/blob/29c4509a5b609ddf7d0706a69d213f6339bad161/spark/src/main/scala/org/apache/comet/serde/arithmetic.scala#L414-L428) | `round` | `negativeScaleReason` | | [`CometCeil`, `CometFloor`](https://github.com/apache/datafusion-comet/blob/29c4509a5b609ddf7d0706a69d213f6339bad161/spark/src/main/scala/org/apache/comet/serde/math.scala#L44-L45) | `ceil`, `floor` | `"Decimal type $t has negative scale"` | The sites have good reasons to route differently. The casts and `round` run in the JVM codegen dispatcher, division falls back to Spark, and the string cast is incompatible unless the legacy flag is set. They have no reason to differ in how they detect a negative scale or how they describe it. Spread out like this, a missing case is easy to overlook. The first review of #5050 found the boolean-to-decimal cast unguarded, and the list keeps growing. ### Describe the potential solution Add one predicate, such as `isNegativeScaleDecimal(dt: DataType)`, and one reason string, and use them at every site above, keeping each site's routing as it is. The generated compatibility pages would then describe the restriction the same way everywhere. A single check over the whole plan, or one in `exprToProtoInternal`, also came up on #5050. Either would fall back for operations #5050 deliberately keeps native, such as `+`, `-`, `*`, `%`, unary minus, and the float, double, string and boolean casts, so a shared predicate at the existing sites fits better. ### Additional context Line references are pinned to #5050's head, 29c4509a5b. -- 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]
