Support timestamp type in mysql jdbc connector #21937

hantangwangd · 2024-02-15T03:14:17Z

Description

Fix issue #21731. Support column type: timestamp in mysql connector.

Test Plan

Newly added test case in TestMySqlIntegrationSmokeTest.testMysqlTimestamp()

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

== RELEASE NOTES ==

Mysql Changes
* Support timestamp column type

yhwang · 2024-02-15T23:09:20Z

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcPageSink.java

@@ -165,6 +169,22 @@ else if (DATE.equals(type)) {
            long localMillis = getInstanceUTC().getZone().getMillisKeepLocal(DateTimeZone.getDefault(), utcMillis);
            statement.setDate(parameter, new Date(localMillis));
        }
+        else if (type instanceof TimestampType) {
+            long timestampValue = type.getLong(block, position);
+            int nanos = ((TimestampType) type).getPrecision() == MILLISECONDS ?


Just thinking would it make sense to move get nanos and get millis to com.facebook.presto.common.type.TimestampType although this would be the only place to use them.

Good idea! These logics are common for timestamp and may be used somewhere else.

To make it more universal, I have extracted getEpochSecond(long timestamp) which returns timestamp's number of total seconds, and getNanos(long timestamp) which returns the timestamp's nanosecond portion. Please take a look. Any thoughts please let me know, thanks!

yhwang · 2024-02-16T01:35:22Z

presto-common/src/main/java/com/facebook/presto/common/type/TimestampType.java

+     * Returns:
+     * this timestamp's fractional seconds component
+     */
+    public int getNanos(long timestamp)


just curious that any reason you return int here instead of long. I saw the API for Instant.ofEpochSecond is (long, long).

Another thing I found is that java.util.concurrent.TimeUnit provides MICROSECONDS.toNanos(long) and MILLISECONDS.toNanos(long) not sure if that's relevant.

just curious that any reason you return int here instead of long. I saw the API for Instant.ofEpochSecond is (long, long).

I consulted the API definition of java.time.Instant.getNano(), int type would be enough as the return value of getNanos would always less than 1000,000,000. What's your opinion?

Another thing I found is that java.util.concurrent.TimeUnit provides MICROSECONDS.toNanos(long) and MILLISECONDS.toNanos(long) not sure if that's relevant.

Thanks, that could be useful. Consulting the implementation in DecodeTimestampOptions, I have modified the code to use them. Please take a look!

By the way, seems the second parameter of Instant.ofEpochSecond(long, long) could be nanos representing longer than one second, so its type was declared as long.

Thanks for the explanation. I believe for the current usage, it only reduces the stack size but not the heap because java.sql.Timestamp is still using long and consumes the same amount of heap size.

However, if this method is used by other code and the constructed object is using int, then it does reduce the heap usage. You can leave it as int.

yhwang · 2024-02-16T03:45:19Z

presto-common/src/main/java/com/facebook/presto/common/type/TimestampType.java

+    {
+        return this.precision == MILLISECONDS ?
+                MILLISECONDS.toSeconds(timestamp) :
+                MICROSECONDS.toSeconds(timestamp);


How about this.precision.toSeconds(timestamp)?
If that's the case, I start to wonder do we need this function. sorry for the hassle.

How about this.precision.toSeconds(timestamp)?

Oh, it's better, I have fixed it. Thanks a lot for your suggestion that lead to the continuous improvement in our implementation.

I think this function still has some value for providing the utility logic and the corresponding comment, as the usage TimestampType.getPrecision().toSeconds(long) from other place is not so straightforward. What do you think?

sure! sold.

yhwang · 2024-02-16T03:52:40Z

presto-common/src/main/java/com/facebook/presto/common/type/TimestampType.java

+     * Returns:
+     * this timestamp's fractional seconds component
+     */
+    public int getNanos(long timestamp)


Thanks for the explanation. I believe for the current usage, it only reduces the stack size but not the heap because java.sql.Timestamp is still using long and consumes the same amount of heap size.

However, if this method is used by other code and the constructed object is using int, then it does reduce the heap usage. You can leave it as int.

kiersten-stokes

LGTM!

pratyakshsharma · 2024-07-03T08:52:25Z

It looks like this PR only supports milliseconds precision for timestamp type. I will raise a github issue and link with this PR.

hantangwangd · 2024-07-04T00:35:09Z

It looks like this PR only supports milliseconds precision for timestamp type. I will raise a github issue and link with this PR.

@pratyakshsharma Sure, thank you for pointing out the problems if there is any. I think this change can internally support microseconds precision as well. So please describe the problematic scenario in the new issue.

hantangwangd requested a review from a team as a code owner February 15, 2024 03:14

hantangwangd requested a review from presto-oss February 15, 2024 03:14

hantangwangd linked an issue Feb 15, 2024 that may be closed by this pull request

Query 20240119_063836_00856_pugph failed: Unsupported column type: timestamp #21731

Closed

hantangwangd force-pushed the support_timestamp_in_mysql branch from 20da0e1 to b0ff5f5 Compare February 15, 2024 04:12

hantangwangd requested review from yhwang, kiersten-stokes and tdcmeehan February 15, 2024 05:14

yhwang reviewed Feb 15, 2024

View reviewed changes

tdcmeehan self-assigned this Feb 15, 2024

hantangwangd force-pushed the support_timestamp_in_mysql branch from b0ff5f5 to ad06658 Compare February 16, 2024 00:50

yhwang reviewed Feb 16, 2024

View reviewed changes

hantangwangd force-pushed the support_timestamp_in_mysql branch from ad06658 to 9fec3b9 Compare February 16, 2024 02:09

yhwang reviewed Feb 16, 2024

View reviewed changes

Support timestamp type in mysql jdbc connector

86cad4d

hantangwangd force-pushed the support_timestamp_in_mysql branch from 9fec3b9 to 86cad4d Compare February 16, 2024 09:31

yhwang approved these changes Feb 16, 2024

View reviewed changes

kiersten-stokes approved these changes Feb 20, 2024

View reviewed changes

tdcmeehan approved these changes Feb 21, 2024

View reviewed changes

tdcmeehan merged commit 91b0bb3 into prestodb:master Feb 21, 2024
56 checks passed

wanglinsong mentioned this pull request May 1, 2024

Add release notes for 0.287 #22647

Merged

48 tasks

pratyakshsharma mentioned this pull request Jul 4, 2024

Microseconds precision is not respected in mysql connector #23133

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support timestamp type in mysql jdbc connector #21937

Support timestamp type in mysql jdbc connector #21937

hantangwangd commented Feb 15, 2024 •

edited

Loading

yhwang Feb 15, 2024

hantangwangd Feb 16, 2024

yhwang Feb 16, 2024

hantangwangd Feb 16, 2024 •

edited

Loading

hantangwangd Feb 16, 2024

yhwang Feb 16, 2024

yhwang Feb 16, 2024

hantangwangd Feb 16, 2024 •

edited

Loading

yhwang Feb 16, 2024

yhwang Feb 16, 2024

kiersten-stokes left a comment

pratyakshsharma commented Jul 3, 2024

hantangwangd commented Jul 4, 2024

Support timestamp type in mysql jdbc connector #21937

Support timestamp type in mysql jdbc connector #21937

Conversation

hantangwangd commented Feb 15, 2024 • edited Loading

Description

Test Plan

Contributor checklist

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hantangwangd Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hantangwangd Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiersten-stokes left a comment

Choose a reason for hiding this comment

pratyakshsharma commented Jul 3, 2024

hantangwangd commented Jul 4, 2024

hantangwangd commented Feb 15, 2024 •

edited

Loading

hantangwangd Feb 16, 2024 •

edited

Loading

hantangwangd Feb 16, 2024 •

edited

Loading