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/05/18 12:34:29 UTC
[GitHub] [spark] GuoPhilipse opened a new pull request #28571: Add compatibility flag to cast long to timestamp
GuoPhilipse opened a new pull request #28571:
URL: https://github.com/apache/spark/pull/28571
What changes were proposed in this pull request?
As we know,long datatype is interpreted as milliseconds when conversion to timestamp in hive, while long is interpreted as seconds when conversion to timestamp in spark, we have been facing error data during migrating hive sql to spark sql. with compatibility flag we can fix this error,
Why are the changes needed?
we have many sqls runing in product, so we need a compatibility flag to make them migrating smoothly ,meanwhile do not change the user behavior in spark.
Does this PR introduce any user-facing change?
if user use this patch ,then user should set this paramter ,
if not, user do not need to do anything.
How was this patch tested?
unit test added
----------------------------------------------------------------
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 #28571: Add compatibility flag to cast long to timestamp
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28571:
URL: https://github.com/apache/spark/pull/28571#issuecomment-630153700
Can one of the admins verify this patch?
----------------------------------------------------------------
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 #28571: Add compatibility flag to cast long to timestamp
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28571:
URL: https://github.com/apache/spark/pull/28571#issuecomment-630153210
Can one of the admins verify this patch?
----------------------------------------------------------------
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] HyukjinKwon commented on a change in pull request #28571: Add compatibility flag to cast long to timestamp
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #28571:
URL: https://github.com/apache/spark/pull/28571#discussion_r426998182
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -390,8 +390,12 @@ case class Cast(child: Expression, dataType: DataType, timeZoneId: Option[String
if (d.isNaN || d.isInfinite) null else (d * 1000000L).toLong
}
+ // SPARK-31710.Add compatibility flag to cast long to timestamp,
// converting seconds to us
- private[this] def longToTimestamp(t: Long): Long = t * 1000000L
+ private[this] def longToTimestamp(t: Long): Long = {
+ if (SQLConf.get.getConf(SQLConf.LONG_TIMESTAMP_CONVERSION_IN_SECONDS)) t * 1000000L
Review comment:
Let's don't do this. It will require to add the compatibility across all the date time functionality in Spark, e.g.) `from_unixtime`.
----------------------------------------------------------------
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 #28571: Add compatibility flag to cast long to timestamp
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28571:
URL: https://github.com/apache/spark/pull/28571#issuecomment-630153210
Can one of the admins verify this patch?
----------------------------------------------------------------
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] GuoPhilipse commented on pull request #28571: Add compatibility flag to cast long to timestamp
Posted by GitBox <gi...@apache.org>.
GuoPhilipse commented on pull request #28571:
URL: https://github.com/apache/spark/pull/28571#issuecomment-630503356
thanks Takeshi ,i will read the rules.
At 2020-05-19 07:43:14, "Takeshi Yamamuro" <no...@github.com> wrote:
Hi, @GuoPhilipse. Could you pelase read the Pull Request section in the our contribution guide first? Then, plz follow the rules there. https://spark.apache.org/contributing.html Thanks for your contribution, anyway.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
----------------------------------------------------------------
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] HyukjinKwon commented on pull request #28571: Add compatibility flag to cast long to timestamp
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28571:
URL: https://github.com/apache/spark/pull/28571#issuecomment-630562207
@GuoPhilipse, it's a matter of coherence in Apache Spark instead of fixing this one case alone. Also, you should open a PR against the master branch if you want to change something newly.
----------------------------------------------------------------
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] HyukjinKwon closed pull request #28571: Add compatibility flag to cast long to timestamp
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #28571:
URL: https://github.com/apache/spark/pull/28571
----------------------------------------------------------------
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] maropu commented on pull request #28571: Add compatibility flag to cast long to timestamp
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #28571:
URL: https://github.com/apache/spark/pull/28571#issuecomment-630491322
Hi, @GuoPhilipse. Could you pelase read the `Pull Request` section in the our contribution guide first? Then, plz follow the rules there. https://spark.apache.org/contributing.html Thanks for your contribution, anyway.
----------------------------------------------------------------
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] GuoPhilipse commented on pull request #28571: Add compatibility flag to cast long to timestamp
Posted by GitBox <gi...@apache.org>.
GuoPhilipse commented on pull request #28571:
URL: https://github.com/apache/spark/pull/28571#issuecomment-630558198
hi HyukjinKwon
it will take effect only when converting long to timestamp, other cases currently seems no need to change. we want to keep compatibility back with hive sql,
Do you have any cases? we can discuss more.
Thanls.
At 2020-05-19 10:44:13, "Hyukjin Kwon" <no...@github.com> wrote:
@HyukjinKwon commented on this pull request.
In sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
> // converting seconds to us
- private[this] def longToTimestamp(t: Long): Long = t * 1000000L
+ private[this] def longToTimestamp(t: Long): Long = {
+ if (SQLConf.get.getConf(SQLConf.LONG_TIMESTAMP_CONVERSION_IN_SECONDS)) t * 1000000L
Let's don't do this. It will require to add the compatibility across all the date time functionality in Spark, e.g.) from_unixtime.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
----------------------------------------------------------------
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