[GitHub] [spark] cloud-fan commented on a change in pull request #27707: [SPARK-30958][SQL] do not set default era for DateTimeFormatter
cloud-fan commented on a change in pull request #27707: [SPARK-30958][SQL] do
not set default era for DateTimeFormatter
URL: https://github.com/apache/spark/pull/27707#discussion_r384908202
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##
@@ -20,26 +20,26 @@ package org.apache.spark.sql.catalyst.util
import java.time._
import java.time.chrono.IsoChronology
import java.time.format.{DateTimeFormatter, DateTimeFormatterBuilder,
ResolverStyle}
-import java.time.temporal.{ChronoField, TemporalAccessor, TemporalQueries}
+import java.time.temporal.{ChronoField, TemporalAccessor}
+import java.time.temporal.ChronoField.{DAY_OF_MONTH, ERA, MONTH_OF_YEAR, YEAR,
YEAR_OF_ERA}
import java.util.Locale
import com.google.common.cache.CacheBuilder
-import org.apache.spark.sql.catalyst.util.DateTimeFormatterHelper._
-
trait DateTimeFormatterHelper {
- // Converts the parsed temporal object to ZonedDateTime. It sets time
components to zeros
- // if they does not exist in the parsed object.
- protected def toZonedDateTime(
- temporalAccessor: TemporalAccessor,
- zoneId: ZoneId): ZonedDateTime = {
-// Parsed input might not have time related part. In that case, time
component is set to zeros.
-val parsedLocalTime = temporalAccessor.query(TemporalQueries.localTime)
-val localTime = if (parsedLocalTime == null) LocalTime.MIDNIGHT else
parsedLocalTime
-// Parsed input must have date component. At least, year must present in
temporalAccessor.
-val localDate = temporalAccessor.query(TemporalQueries.localDate)
+ import DateTimeFormatterHelper._
-ZonedDateTime.of(localDate, localTime, zoneId)
+ protected def getLocalDate(str: String, parsed: TemporalAccessor): LocalDate
= {
+val year = if (parsed.isSupported(YEAR)) {
+ parsed.get(YEAR)
+} else if (parsed.isSupported(YEAR_OF_ERA) && !parsed.isSupported(ERA)) {
+ // This can happen when the year pattern is "" without "G" to
specify era. We assume the
+ // era is AC, and just treat YEAR_OF_ERA as YEAR.
+ parsed.get(YEAR_OF_ERA)
+} else {
+ throw new DateTimeException("Cannot obtain year from timestamp string "
+ str)
+}
+LocalDate.of(year, parsed.get(MONTH_OF_YEAR), parsed.get(DAY_OF_MONTH))
Review comment:
The main change is here: instead of doing
`temporalAccessor.query(TemporalQueries.localDate)`, we can get the date fields
manually, then we have a chance to handle the missing era.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27707: [SPARK-30958][SQL] do not set default era for DateTimeFormatter
cloud-fan commented on a change in pull request #27707: [SPARK-30958][SQL] do
not set default era for DateTimeFormatter
URL: https://github.com/apache/spark/pull/27707#discussion_r384649943
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/TimestampFormatter.scala
##
@@ -62,10 +62,22 @@ class Iso8601TimestampFormatter(
val parsed = formatter.parse(s)
val parsedZoneId = parsed.query(TemporalQueries.zone())
val timeZoneId = if (parsedZoneId == null) zoneId else parsedZoneId
- val zonedDateTime = toZonedDateTime(parsed, timeZoneId)
+ val year = if (parsed.isSupported(YEAR)) {
+parsed.get(YEAR)
+ } else if (parsed.isSupported(YEAR_OF_ERA) && !parsed.isSupported(ERA)) {
+// This means the year pattern is "" without "G" to specify era.
+parsed.get(YEAR_OF_ERA)
+ } else {
+throw new DateTimeException("Cannot obtain year from timestamp string
" + s)
+ }
+ // Parsed input might not have time related part. In that case, time
component is set to
+ // zeros.
+ val parsedLocalTime = parsed.query(TemporalQueries.localTime)
+ val localTime = if (parsedLocalTime == null) LocalTime.MIDNIGHT else
parsedLocalTime
+ val localDate = LocalDate.of(year, parsed.get(MONTH_OF_YEAR),
parsed.get(DAY_OF_MONTH))
Review comment:
The main change is here: instead of doing
`temporalAccessor.query(TemporalQueries.localDate)`, we can get the date
fields manually, then we have a chance to handle the missing era.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27707: [SPARK-30958][SQL] do not set default era for DateTimeFormatter
cloud-fan commented on a change in pull request #27707: [SPARK-30958][SQL] do
not set default era for DateTimeFormatter
URL: https://github.com/apache/spark/pull/27707#discussion_r384649943
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/TimestampFormatter.scala
##
@@ -62,10 +62,22 @@ class Iso8601TimestampFormatter(
val parsed = formatter.parse(s)
val parsedZoneId = parsed.query(TemporalQueries.zone())
val timeZoneId = if (parsedZoneId == null) zoneId else parsedZoneId
- val zonedDateTime = toZonedDateTime(parsed, timeZoneId)
+ val year = if (parsed.isSupported(YEAR)) {
+parsed.get(YEAR)
+ } else if (parsed.isSupported(YEAR_OF_ERA) && !parsed.isSupported(ERA)) {
+// This means the year pattern is "" without "G" to specify era.
+parsed.get(YEAR_OF_ERA)
+ } else {
+throw new DateTimeException("Cannot obtain year from timestamp string
" + s)
+ }
+ // Parsed input might not have time related part. In that case, time
component is set to
+ // zeros.
+ val parsedLocalTime = parsed.query(TemporalQueries.localTime)
+ val localTime = if (parsedLocalTime == null) LocalTime.MIDNIGHT else
parsedLocalTime
+ val localDate = LocalDate.of(year, parsed.get(MONTH_OF_YEAR),
parsed.get(DAY_OF_MONTH))
Review comment:
The main change is here: instead of doing
`temporalAccessor.query(TemporalQueries.localDate)`, we can get the date
fields manually, then we have a chance to handle the missing era.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27707: [SPARK-30958][SQL] do not set default era for DateTimeFormatter
cloud-fan commented on a change in pull request #27707: [SPARK-30958][SQL] do
not set default era for DateTimeFormatter
URL: https://github.com/apache/spark/pull/27707#discussion_r384648455
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##
@@ -82,7 +81,14 @@ private object DateTimeFormatterHelper {
def buildFormatter(pattern: String, locale: Locale): DateTimeFormatter = {
val builder = createBuilder().appendPattern(pattern)
-toFormatter(builder, locale)
+if (pattern.contains("y")) {
Review comment:
this doesn't work, as the type `DateTimePrinterParser` is not accessible
outside.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #27707: [SPARK-30958][SQL] do not set default era for DateTimeFormatter
cloud-fan commented on a change in pull request #27707: [SPARK-30958][SQL] do
not set default era for DateTimeFormatter
URL: https://github.com/apache/spark/pull/27707#discussion_r384432888
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##
@@ -82,7 +81,14 @@ private object DateTimeFormatterHelper {
def buildFormatter(pattern: String, locale: Locale): DateTimeFormatter = {
val builder = createBuilder().appendPattern(pattern)
-toFormatter(builder, locale)
+if (pattern.contains("y")) {
Review comment:
good catch! we should use regex or write custom parsing code.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
