yaooqinn commented on a change in pull request #28650:
URL: https://github.com/apache/spark/pull/28650#discussion_r437232828



##########
File path: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala
##########
@@ -56,6 +54,26 @@ trait TimeZoneAwareExpression extends Expression {
   @transient lazy val zoneId: ZoneId = DateTimeUtils.getZoneId(timeZoneId.get)
 }
 
+trait TimestampFormatterHelper extends TimeZoneAwareExpression {

Review comment:
       no datetime functions, only csv/json ones

##########
File path: sql/core/src/test/resources/sql-tests/inputs/datetime.sql
##########
@@ -138,25 +138,11 @@ select to_timestamp("2019 40", "yyyy mm");
 select to_timestamp("2019 10:10:10", "yyyy hh:mm:ss");
 
 -- Unsupported narrow text style
-select date_format(date '2020-05-23', 'GGGGG');

Review comment:
       `datetime-formatting-invalid.sql` should be enough to cover these cases

##########
File path: sql/core/src/test/scala/org/apache/spark/sql/DateFunctionsSuite.scala
##########
@@ -689,8 +689,9 @@ class DateFunctionsSuite extends QueryTest with 
SharedSparkSession {
           Row(secs(ts5.getTime)), Row(null)))
 
         // invalid format
-        checkAnswer(df1.selectExpr(s"to_unix_timestamp(x, 'yyyy-MM-dd 
bb:HH:ss')"), Seq(
-          Row(null), Row(null), Row(null), Row(null)))
+        val invalid = df1.selectExpr(s"to_unix_timestamp(x, 'yyyy-MM-dd 
bb:HH:ss')")
+        val e = intercept[IllegalArgumentException](invalid.collect())
+        assert(e.getMessage.contains('b'))

Review comment:
       Just pick the intersection of `Illegal pattern character 'b'` and  
`Unknown pattern letter: b`

##########
File path: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala
##########
@@ -1044,7 +1009,8 @@ abstract class UnixTime extends ToTimestamp {
   since = "1.5.0")
 // scalastyle:on line.size.limit
 case class FromUnixTime(sec: Expression, format: Expression, timeZoneId: 
Option[String] = None)
-  extends BinaryExpression with TimeZoneAwareExpression with 
ImplicitCastInputTypes {
+  extends BinaryExpression with TimestampFormatterHelper with 
ImplicitCastInputTypes
+    with NullIntolerant {

Review comment:
       Copy that.




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



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to