HyukjinKwon commented on a change in pull request #33293:
URL: https://github.com/apache/spark/pull/33293#discussion_r667793021
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
##########
@@ -237,114 +237,114 @@ object DateTimeUtils {
* - Region-based zone IDs in the form `area/city`, such as `Europe/Paris`
*/
def stringToTimestamp(s: UTF8String, timeZoneId: ZoneId): Option[Long] = {
- if (s == null) {
- return None
- }
- var tz: Option[String] = None
- val segments: Array[Int] = Array[Int](1, 1, 1, 0, 0, 0, 0, 0, 0)
- var i = 0
- var currentSegmentValue = 0
- val bytes = s.trimAll().getBytes
- val specialTimestamp = convertSpecialTimestamp(bytes, timeZoneId)
- if (specialTimestamp.isDefined) return specialTimestamp
- var j = 0
- var digitsMilli = 0
- var justTime = false
- while (j < bytes.length) {
- val b = bytes(j)
- val parsedValue = b - '0'.toByte
- if (parsedValue < 0 || parsedValue > 9) {
- if (j == 0 && b == 'T') {
- justTime = true
- i += 3
- } else if (i < 2) {
- if (b == '-') {
- if (i == 0 && j != 4) {
- // year should have exact four digits
+ try {
Review comment:
Why should we make the whole block with try-catch? can we only do that
for problematic block only?
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
##########
@@ -237,114 +237,114 @@ object DateTimeUtils {
* - Region-based zone IDs in the form `area/city`, such as `Europe/Paris`
*/
def stringToTimestamp(s: UTF8String, timeZoneId: ZoneId): Option[Long] = {
- if (s == null) {
- return None
- }
- var tz: Option[String] = None
- val segments: Array[Int] = Array[Int](1, 1, 1, 0, 0, 0, 0, 0, 0)
- var i = 0
- var currentSegmentValue = 0
- val bytes = s.trimAll().getBytes
- val specialTimestamp = convertSpecialTimestamp(bytes, timeZoneId)
- if (specialTimestamp.isDefined) return specialTimestamp
- var j = 0
- var digitsMilli = 0
- var justTime = false
- while (j < bytes.length) {
- val b = bytes(j)
- val parsedValue = b - '0'.toByte
- if (parsedValue < 0 || parsedValue > 9) {
- if (j == 0 && b == 'T') {
- justTime = true
- i += 3
- } else if (i < 2) {
- if (b == '-') {
- if (i == 0 && j != 4) {
- // year should have exact four digits
+ try {
Review comment:
no the original function is not in try-catch. The specific function is
factored out and wrapped with try-catch, isn't it?
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
##########
@@ -237,114 +237,114 @@ object DateTimeUtils {
* - Region-based zone IDs in the form `area/city`, such as `Europe/Paris`
*/
def stringToTimestamp(s: UTF8String, timeZoneId: ZoneId): Option[Long] = {
Review comment:
Can we make the change minimised? you can do, for example,
```suggestion
def stringToTimestamp(s: UTF8String, timeZoneId: ZoneId): Option[Long] =
try {
```
--
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
