Skip to content

Conversation

@beliefer
Copy link
Contributor

@beliefer beliefer commented May 5, 2023

What changes were proposed in this pull request?

#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.

The micro benchmark.

Java HotSpot(TM) 64-Bit Server VM 1.8.0_311-b11 on Mac OS X 10.16
Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
Compare timestamp:                          Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
--------------------------------------------------------------------------------------------------------------------------
Before: convertJavaTimestampToTimestampNTZ              6              7           2         16.8          59.6       1.0X
After: convertJavaTimestampToTimestampNTZ               1              1           0        188.3           5.3      11.2X

@github-actions github-actions bot added the SQL label May 5, 2023
@beliefer
Copy link
Contributor Author

beliefer commented May 5, 2023

ping @cloud-fan

@Since("3.5.0")
def convertJavaTimestampToTimestampNTZ(t: Timestamp): LocalDateTime = {
DateTimeUtils.microsToLocalDateTime(DateTimeUtils.fromJavaTimestampNoRebase(t))
def convertJavaTimestampToTimestampNTZ(t: Timestamp): Long = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. the performance is very bad.

@beliefer beliefer closed this May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants