JoshRosen commented on a change in pull request #25010: [SPARK-28201][SQL]
Revisit MakeDecimal behavior on overflow
URL: https://github.com/apache/spark/pull/25010#discussion_r298804025
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/decimalExpressions.scala
##########
@@ -46,19 +47,35 @@ case class UnscaledValue(child: Expression) extends
UnaryExpression {
*/
case class MakeDecimal(child: Expression, precision: Int, scale: Int) extends
UnaryExpression {
+ private val nullOnOverflow = SQLConf.get.decimalOperationsNullOnOverflow
+ private lazy val doEval = if (nullOnOverflow) {
Review comment:
Is there a performance advantage to caching the result of the
`nullOnOverflow` check instead of putting an `if-else` in `def nullSafeEval`
itself?
My intuition (possibly wrong) is that a method call indirecting through
`doEval` is going to be more expensive than a simple boolean check inside the
body of `nullSafeEval` (or at least comparable).
If we don't have a strong motivation for this approach, I have a preference
for the simpler:
```scala
protected override def nullSafeEval(input: Any): Any = {
if (nullOnOverflow) {
new Decimal().setOrNull(input.asInstanceOf[Long], precision, scale)
} else {
new Decimal().set(input.asInstanceOf[Long], precision, scale)
}
}
```
because it more closely matches the `doGenCode`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
