uros-db commented on code in PR #51824: URL: https://github.com/apache/spark/pull/51824#discussion_r2297282264
########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala: ########## @@ -3003,40 +3003,30 @@ case class MakeTimestamp( } } -// scalastyle:off line.size.limit -@ExpressionDescription( - usage = "_FUNC_(year, month, day, hour, min, sec[, timezone]) - Try to create a timestamp from year, month, day, hour, min, sec and timezone fields. The result data type is consistent with the value of configuration `spark.sql.timestampType`. The function returns NULL on invalid inputs.", - arguments = """ - Arguments: - * year - the year to represent, from 1 to 9999 - * month - the month-of-year to represent, from 1 (January) to 12 (December) - * day - the day-of-month to represent, from 1 to 31 - * hour - the hour-of-day to represent, from 0 to 23 - * min - the minute-of-hour to represent, from 0 to 59 - * sec - the second-of-minute and its micro-fraction to represent, from 0 to 60. - The value can be either an integer like 13 , or a fraction like 13.123. - If the sec argument equals to 60, the seconds field is set - to 0 and 1 minute is added to the final timestamp. - * timezone - the time zone identifier. For example, CET, UTC and etc. - """, - examples = """ - Examples: - > SELECT _FUNC_(2014, 12, 28, 6, 30, 45.887); - 2014-12-28 06:30:45.887 - > SELECT _FUNC_(2014, 12, 28, 6, 30, 45.887, 'CET'); - 2014-12-27 21:30:45.887 - > SELECT _FUNC_(2019, 6, 30, 23, 59, 60); - 2019-07-01 00:00:00 - > SELECT _FUNC_(2019, 6, 30, 23, 59, 1); - 2019-06-30 23:59:01 - > SELECT _FUNC_(null, 7, 22, 15, 30, 0); - NULL - > SELECT _FUNC_(2024, 13, 22, 15, 30, 0); - NULL - """, - group = "datetime_funcs", - since = "4.0.0") -// scalastyle:on line.size.limit +case class TryMakeTimestampFromDateTime( Review Comment: The first comment is indeed true, although I would argue that these tests shouldn't hurt as they currently are in this PR. Test coverage for date/time related functions is not great across the codebase, so I would suggest to at least keep these tests for `try_make_timestamp`. They shouldn't really slow down our overall CI by any considerable amount. What do you think? Also, no hard opinion here - I see your point and I can remove some tests, if that's needed. -- 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
