You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/06/22 08:34:15 UTC
[GitHub] [spark] MaxGekk opened a new pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
MaxGekk opened a new pull request #28892:
URL: https://github.com/apache/spark/pull/28892
### What changes were proposed in this pull request?
Call the `replace()` method from `UTF8String` to remove the `GMT` string from the input of `DateTimeUtils.cleanLegacyTimestampStr`. It removes all `GMT` substrings.
### Why are the changes needed?
Simpler code improves maintainability
### Does this PR introduce _any_ user-facing change?
Should not
### How was this patch tested?
By existing test suites `JsonSuite` and `UnivocityParserSuite`.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647527952
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647573299
**[Test build #124359 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124359/testReport)** for PR 28892 at commit [`bcea391`](https://github.com/apache/spark/commit/bcea39121a36c6ac97740f6398f96f024d02b57a).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647371571
**[Test build #124358 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124358/testReport)** for PR 28892 at commit [`c1e4d0c`](https://github.com/apache/spark/commit/c1e4d0c995bcd78f0144bcfbabd383f59d3c5e17).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647378994
**[Test build #124359 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124359/testReport)** for PR 28892 at commit [`bcea391`](https://github.com/apache/spark/commit/bcea39121a36c6ac97740f6398f96f024d02b57a).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28892:
URL: https://github.com/apache/spark/pull/28892#discussion_r443959870
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
##########
@@ -203,20 +203,10 @@ object DateTimeUtils {
Math.multiplyExact(millis, MICROS_PER_MILLIS)
}
+ private final val gmtUtf8 = UTF8String.fromString("GMT")
// The method is called by JSON/CSV parser to clean up the legacy timestamp string by removing
- // the "GMT" string.
- def cleanLegacyTimestampStr(s: String): String = {
- val indexOfGMT = s.indexOf("GMT")
- if (indexOfGMT != -1) {
- // ISO8601 with a weird time zone specifier (2000-01-01T00:00GMT+01:00)
- val s0 = s.substring(0, indexOfGMT)
- val s1 = s.substring(indexOfGMT + 3)
- // Mapped to 2000-01-01T00:00+01:00
- s0 + s1
- } else {
- s
- }
- }
+ // the "GMT" string. For example, it returns 2000-01-01T00:00+01:00 for 2000-01-01T00:00GMT+01:00.
+ def cleanLegacyTimestampStr(s: UTF8String): UTF8String = s.replace(gmtUtf8, UTF8String.EMPTY_UTF8)
Review comment:
doesn't the `java.lang.String` have the `replace` method?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647666509
@srowen @dongjoon-hyun @cloud-fan @HyukjinKwon Could you review the little refactoring.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647924824
thanks, merging to master!
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647379638
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647575097
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a change in pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28892:
URL: https://github.com/apache/spark/pull/28892#discussion_r443973612
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
##########
@@ -203,20 +203,10 @@ object DateTimeUtils {
Math.multiplyExact(millis, MICROS_PER_MILLIS)
}
+ private final val gmtUtf8 = UTF8String.fromString("GMT")
// The method is called by JSON/CSV parser to clean up the legacy timestamp string by removing
- // the "GMT" string.
- def cleanLegacyTimestampStr(s: String): String = {
- val indexOfGMT = s.indexOf("GMT")
- if (indexOfGMT != -1) {
- // ISO8601 with a weird time zone specifier (2000-01-01T00:00GMT+01:00)
- val s0 = s.substring(0, indexOfGMT)
- val s1 = s.substring(indexOfGMT + 3)
- // Mapped to 2000-01-01T00:00+01:00
- s0 + s1
- } else {
- s
- }
- }
+ // the "GMT" string. For example, it returns 2000-01-01T00:00+01:00 for 2000-01-01T00:00GMT+01:00.
+ def cleanLegacyTimestampStr(s: UTF8String): UTF8String = s.replace(gmtUtf8, UTF8String.EMPTY_UTF8)
Review comment:
It has but look at how it is implemented via regexp. @JoshRosen implemented more effective replace in UTF8String https://github.com/apache/spark/pull/24707. That's why I took it. I hope it seems reasonable.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647526408
**[Test build #124358 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124358/testReport)** for PR 28892 at commit [`c1e4d0c`](https://github.com/apache/spark/commit/c1e4d0c995bcd78f0144bcfbabd383f59d3c5e17).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647371571
**[Test build #124358 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124358/testReport)** for PR 28892 at commit [`c1e4d0c`](https://github.com/apache/spark/commit/c1e4d0c995bcd78f0144bcfbabd383f59d3c5e17).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647527952
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647372095
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #28892:
URL: https://github.com/apache/spark/pull/28892
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647378994
**[Test build #124359 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124359/testReport)** for PR 28892 at commit [`bcea391`](https://github.com/apache/spark/commit/bcea39121a36c6ac97740f6398f96f024d02b57a).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647575097
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647372095
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28892: [MINOR][SQL] Simplify DateTimeUtils.cleanLegacyTimestampStr
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28892:
URL: https://github.com/apache/spark/pull/28892#issuecomment-647379638
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org