bersprockets commented on code in PR #38635:
URL: https://github.com/apache/spark/pull/38635#discussion_r1020798321
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/numberFormatExpressions.scala:
##########
@@ -26,6 +26,62 @@ import org.apache.spark.sql.catalyst.util.ToNumberParser
import org.apache.spark.sql.types.{AbstractDataType, DataType, Decimal,
DecimalType, StringType}
import org.apache.spark.unsafe.types.UTF8String
+abstract class ToNumberBase(left: Expression, right: Expression, errorOnFail:
Boolean)
Review Comment:
I moved common code to an abstract class to avoid making the same bug fix
twice.
However, that makes it hard to see the actual bug fix, so I can unwind this
change if needed.
The difference between the old `ToNumber` to new `ToNumberBase` is the way
the `numberFormatter` in initialized, how the `dataType` is chosen, the
`checkInputDataTypes` method, and a line in the `doGenCode` method:
```
1,4c1,17
< case class ToNumber(left: Expression, right: Expression)
< extends BinaryExpression with ImplicitCastInputTypes with NullIntolerant
{
< private lazy val numberFormat =
right.eval().toString.toUpperCase(Locale.ROOT)
< private lazy val numberFormatter = new ToNumberParser(numberFormat, true)
---
> abstract class ToNumberBase(left: Expression, right: Expression,
errorOnFail: Boolean)
> extends BinaryExpression with Serializable with ImplicitCastInputTypes
with NullIntolerant {
>
> private lazy val numberFormatter = {
> val value = right.eval()
> if (value != null) {
> new ToNumberParser(value.toString.toUpperCase(Locale.ROOT),
errorOnFail)
> } else {
> null
> }
> }
>
> override def dataType: DataType = if (numberFormatter != null) {
> numberFormatter.parsedDecimalType
> } else {
> DecimalType.USER_DEFAULT
> }
6d18
< override def dataType: DataType = numberFormatter.parsedDecimalType
7a20
>
11c24,26
< if (right.foldable) {
---
> if (numberFormatter == null) {
> TypeCheckResult.TypeCheckSuccess
> } else if (right.foldable) {
20c35
< override def prettyName: String = "to_number"
---
>
24a40
>
32c48
< |boolean ${ev.isNull} = ${eval.isNull};
---
> |boolean ${ev.isNull} = ${eval.isNull} || ($builder == null);
39,41d54
< override protected def withNewChildrenInternal(
< newLeft: Expression, newRight: Expression): ToNumber =
< copy(left = newLeft, right = newRight)
```
--
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: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
