[GitHub] [spark] amaliujia commented on a change in pull request #35269: [SPARK-28516][SQL] Data Type Formatting Functions: `to_char`

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

amaliujia commented on a change in pull request #35269:
URL: https://github.com/apache/spark/pull/35269#discussion_r828451504



##########
File path: sql/core/src/test/resources/sql-tests/inputs/postgreSQL/int8.sql
##########
@@ -6,15 +6,15 @@
 -- Test int8 64-bit integers.
 -- https://github.com/postgres/postgres/blob/REL_12_BETA2/src/test/regress/sql/int8.sql
 --
-CREATE TABLE INT8_TBL(q1 bigint, q2 bigint) USING parquet;
+CREATE TABLE INT8_TBL(id int, q1 bigint, q2 bigint) USING parquet;

Review comment:
       Why we need test aginast PostgresSQL? You mentioned your implementation didn't follow PostgresSQL spec?

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/numberFormatExpressions.scala
##########
@@ -54,52 +90,72 @@ import org.apache.spark.unsafe.types.UTF8String
   """,
   since = "3.3.0",
   group = "string_funcs")
-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 NumberFormatter(numberFormat)
+case class ToNumber(left: Expression, right: Expression) extends NumberFormatterBase {
+  val isParse: Boolean = true

Review comment:
       I am a bit favor use a function to get the method name in codegen than relying on `isParse`. `isParse` can only be yes or no but say we could have a third implementation extends `NumberFormatterBase`. At that time `isParse` won't be sufficient.
   
   Also `isParse` is also a bit vague. 




-- 
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