You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "beliefer (via GitHub)" <gi...@apache.org> on 2023/05/05 05:36:46 UTC
[GitHub] [spark] beliefer opened a new pull request, #41058: [SPARK-43040][SQL][FOLLOWUP] Avoid duplicated conversion
beliefer opened a new pull request, #41058:
URL: https://github.com/apache/spark/pull/41058
### What changes were proposed in this pull request?
https://github.com/apache/spark/pull/40678 adds a dialect parameter to makeGetter for applying dialect specific conversions when reading a Java Timestamp into TimestampNTZType.
But there exists duplicated conversion.
`JdbcDialect` calls `DateTimeUtils.microsToLocalDateTime` first and return `LocalDateTime` to `makeGetter`.
But `makeGetter` calls `DateTimeUtils.localDateTimeToMicros` later.
Personally, I don't think it's necessary to maintain complete symmetry between `convertJavaTimestampToTimestampNTZ` and `convertTimestampNTZToJavaTimestamp`.
### Why are the changes needed?
Simplify code to avoid duplicated conversion.
### Does this PR introduce _any_ user-facing change?
'No'.
New feature.
### How was this patch tested?
Exists test cases.
--
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-unsubscribe@spark.apache.org
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] beliefer closed pull request #41058: [SPARK-43040][SQL][FOLLOWUP] Avoid duplicated conversion
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer closed pull request #41058: [SPARK-43040][SQL][FOLLOWUP] Avoid duplicated conversion
URL: https://github.com/apache/spark/pull/41058
--
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-unsubscribe@spark.apache.org
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] beliefer commented on pull request #41058: [SPARK-43040][SQL][FOLLOWUP] Avoid duplicated conversion
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #41058:
URL: https://github.com/apache/spark/pull/41058#issuecomment-1535945085
ping @cloud-fan
--
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-unsubscribe@spark.apache.org
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 diff in pull request #41058: [SPARK-43040][SQL][FOLLOWUP] Avoid duplicated conversion
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #41058:
URL: https://github.com/apache/spark/pull/41058#discussion_r1186990157
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -111,11 +111,11 @@ abstract class JdbcDialect extends Serializable with Logging {
* JDBC dialects should override this function to provide implementations that suite their
* JDBC drivers.
* @param t Timestamp returned from JDBC driver getTimestamp method.
- * @return A LocalDateTime representing the same wall clock time as the timestamp in database.
+ * @return A Long value representing the same wall clock time as the timestamp in database.
*/
@Since("3.5.0")
- def convertJavaTimestampToTimestampNTZ(t: Timestamp): LocalDateTime = {
- DateTimeUtils.microsToLocalDateTime(DateTimeUtils.fromJavaTimestampNoRebase(t))
+ def convertJavaTimestampToTimestampNTZ(t: Timestamp): Long = {
Review Comment:
I think symmetry API is more important than tiny perf improvements. But if you have benchmark numbers to show this is a big issue, I'm happy to change my mind.
--
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-unsubscribe@spark.apache.org
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] beliefer commented on a diff in pull request #41058: [SPARK-43040][SQL][FOLLOWUP] Avoid duplicated conversion
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #41058:
URL: https://github.com/apache/spark/pull/41058#discussion_r1187131685
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##########
@@ -111,11 +111,11 @@ abstract class JdbcDialect extends Serializable with Logging {
* JDBC dialects should override this function to provide implementations that suite their
* JDBC drivers.
* @param t Timestamp returned from JDBC driver getTimestamp method.
- * @return A LocalDateTime representing the same wall clock time as the timestamp in database.
+ * @return A Long value representing the same wall clock time as the timestamp in database.
*/
@Since("3.5.0")
- def convertJavaTimestampToTimestampNTZ(t: Timestamp): LocalDateTime = {
- DateTimeUtils.microsToLocalDateTime(DateTimeUtils.fromJavaTimestampNoRebase(t))
+ def convertJavaTimestampToTimestampNTZ(t: Timestamp): Long = {
Review Comment:
Yes. the performance is very bad.
--
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-unsubscribe@spark.apache.org
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