[GitHub] [spark] maropu commented on a change in pull request #30440: [SPARK-33496][SQL]Improve error message of ANSI explicit cast

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

maropu commented on a change in pull request #30440:
URL: https://github.com/apache/spark/pull/30440#discussion_r529251244



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -1783,6 +1798,14 @@ case class AnsiCast(child: Expression, dataType: DataType, timeZoneId: Option[St
   override protected val ansiEnabled: Boolean = true
 
   override def canCast(from: DataType, to: DataType): Boolean = AnsiCast.canCast(from, to)
+
+  // For now, this expression is only used in table insertion.
+  // If there are more scenarios for this expression,  we should update the error message on type

Review comment:
       nit: a unnecessary space found in `expression,  we`

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -98,6 +98,19 @@ object Cast {
     case _ => false
   }
 
+  def typeCheckFailureMessage(from: DataType, to: DataType): String = (from, to) match {
+    case (_: NumericType, TimestampType) =>
+      // scalastyle:off line.size.limit
+      s"""
+         | cannot cast ${from.catalogString} to ${to.catalogString},
+         | you can enable the casting by setting ${SQLConf.LEGACY_ALLOW_CAST_NUMERIC_TO_TIMESTAMP.key}
+         | to true, but we strongly recommend using function TIMESTAMP_SECONDS/TIMESTAMP_MILLIS/TIMESTAMP_MICROS instead.

Review comment:
       nit: `function` -> `functions`?

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -1885,6 +1908,35 @@ object AnsiCast {
 
     case _ => false
   }
+
+  def typeCheckFailureMessage(
+      from: DataType,
+      to: DataType,
+      fallbackConfKey: String,
+      fallbackConfValue: String): String =
+    (from, to) match {
+      case (_: NumericType, TimestampType) =>
+        // scalastyle:off line.size.limit
+        s"""
+           | cannot cast ${from.catalogString} to ${from.catalogString}.

Review comment:
       `from.catalogString` -> `to.catalogString` for the latter one.

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -1885,6 +1908,35 @@ object AnsiCast {
 
     case _ => false
   }
+
+  def typeCheckFailureMessage(
+      from: DataType,
+      to: DataType,
+      fallbackConfKey: String,
+      fallbackConfValue: String): String =
+    (from, to) match {
+      case (_: NumericType, TimestampType) =>
+        // scalastyle:off line.size.limit
+        s"""
+           | cannot cast ${from.catalogString} to ${from.catalogString}.
+           | We strongly recommend using function TIMESTAMP_SECONDS/TIMESTAMP_MILLIS/TIMESTAMP_MICROS instead.

Review comment:
       How about describing it like `To convert values from ${...} to ${...}, you can use functions TIMESTAMP_SECONDS/TIMESTAMP_MILLIS/TIMESTAMP_MICROS instead.` ?




----------------------------------------------------------------
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:
users@infra.apache.org



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