MaxGekk commented on code in PR #50525:
URL: https://github.com/apache/spark/pull/50525#discussion_r2032713539


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -290,3 +290,61 @@ object HourExpressionBuilder extends ExpressionBuilder {
     }
   }
 }
+
+case class SecondsOfTime(child: Expression)
+  extends RuntimeReplaceable
+    with ExpectsInputTypes {
+
+  override def replacement: Expression = StaticInvoke(
+    classOf[DateTimeUtils.type],
+    IntegerType,
+    "getSecondsOfTime",
+    Seq(child),
+    Seq(child.dataType)
+  )
+
+  override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())

Review Comment:
   You allow any precision of `TimeType` in `SecondExpressionBuilder`, but here 
expectsonly `TimeType(6)`. It should work for any valid precision. For now, see:
   ```sql
   spark-sql (default)> select second(cast('12:00:01.123' as time(3)));
   [DATATYPE_MISMATCH.UNEXPECTED_INPUT_TYPE] Cannot resolve 
"second(CAST(12:00:01.123 AS TIME(3)))" due to data type mismatch: The first 
parameter requires the "TIME(6)" type, however "CAST(12:00:01.123 AS TIME(3))" 
has the type "TIME(3)". SQLSTATE: 42K09; line 1 pos 7;
   ```



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -290,3 +290,61 @@ object HourExpressionBuilder extends ExpressionBuilder {
     }
   }
 }
+
+case class SecondsOfTime(child: Expression)
+  extends RuntimeReplaceable
+    with ExpectsInputTypes {
+
+  override def replacement: Expression = StaticInvoke(
+    classOf[DateTimeUtils.type],
+    IntegerType,
+    "getSecondsOfTime",
+    Seq(child),
+    Seq(child.dataType)
+  )
+
+  override def inputTypes: Seq[AbstractDataType] = Seq(TimeType())
+
+  override def children: Seq[Expression] = Seq(child)
+
+  override def prettyName: String = "second"
+
+  override protected def withNewChildrenInternal(
+    newChildren: IndexedSeq[Expression]): Expression = {
+      copy(child = newChildren.head)
+  }
+}
+
+@ExpressionDescription(
+  usage = """
+    _FUNC_(expr) - Returns the second component of the given expression.
+
+    If `expr` is a TIMESTAMP or a string that can be cast to timestamp,
+    it returns the second of that timestamp.
+    If `expr` is a TIME type (since 4.1.0), it returns the second of the 
time-of-day.
+  """,
+  examples = """
+    Examples:
+      > SELECT _FUNC_('2018-02-14 12:58:59');
+       59
+      > SELECT _FUNC_(TIME'13:59:59.999999');

Review Comment:
   It would be better to demonstrate different numbers for seconds and minutes, 
let's say:
   ```
         > SELECT _FUNC_(TIME'13:10:59.999999');
   ```



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/timeExpressions.scala:
##########
@@ -290,3 +290,61 @@ object HourExpressionBuilder extends ExpressionBuilder {
     }
   }
 }
+
+case class SecondsOfTime(child: Expression)
+  extends RuntimeReplaceable
+    with ExpectsInputTypes {
+
+  override def replacement: Expression = StaticInvoke(
+    classOf[DateTimeUtils.type],
+    IntegerType,

Review Comment:
   Yep, the type should be `IntergerType`



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

Reply via email to