[SPARK-27031][SQL] Avoid double formatting in timestampToString#23936
[SPARK-27031][SQL] Avoid double formatting in timestampToString#23936MaxGekk wants to merge 5 commits intoapache:masterfrom
Conversation
srowen
left a comment
srowen
left a comment
There was a problem hiding this comment.
Looks reasonable, and I think you know this part better than anyone. Is there any behavior change?
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/TimestampFormatter.scala
Outdated
Show resolved
Hide resolved
|
Test build #102938 has finished for PR 23936 at commit
|
I believe there shouldn't be. |
|
Test build #102940 has finished for PR 23936 at commit
|
| } | ||
|
|
||
| def getFractionFormatter(timeZone: TimeZone): TimestampFormatter = { | ||
| apply(DateTimeFormatterHelper.fractionPattern, timeZone, defaultLocale) |
There was a problem hiding this comment.
OK, last nit then: this should probably use the naming convention for constants, FRACTION_PATTERN.
How about just a method to return this formatter, rather than a special constant that causes another method to return it?
There was a problem hiding this comment.
Maybe it makes sense to inherit from Iso8601TimestampFormatter and override formatter. I will try and see how it will look like.
There was a problem hiding this comment.
I have done this. Please, take a look at the last commit.
There was a problem hiding this comment.
I have to support only one locale Locale.US but I think it is ok for this case. If we will need the fraction formatter for multiple locales, we can come back to the original solution with caching.
|
Test build #103000 has finished for PR 23936 at commit
|
|
Test build #4592 has finished for PR 23936 at commit
|
| } | ||
| } | ||
|
|
||
| class FractionTimestampFormatter(timeZone: TimeZone) |
There was a problem hiding this comment.
Sorry, last nit. Can you add some docs here?
|
Test build #103052 has finished for PR 23936 at commit
|
|
Merged to master |
4 participants
What changes were proposed in this pull request?
Removed unnecessary conversion of microseconds in
DateTimeUtils.timestampToStringtojava.sql.Timestampwhich aims to output fraction of seconds by casting it to string. This was replaced by specialTimestampFormatterwhich appends the fraction formatter toDateTimeFormatterBuilder:appendFraction(ChronoField.NANO_OF_SECOND, 0, 9, true). The former one means trailing zeros in second's fraction should be truncated while formatting.How was this patch tested?
By existing test suites like
CastSuite,DateTimeUtilsSuite,JDBCSuite, and by new test inTimestampFormatterSuite.