[GitHub] [spark] bersprockets commented on a diff in pull request #38635: [SPARK-41118][SQL] `to_number`/`try_to_number` should return `null` when format is `null`

[GitBox <gi...@apache.org>]

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-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org