Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow time and timestamp inserts via Jdbc #17255

Closed
wants to merge 0 commits into from

Conversation

opatel99
Copy link
Contributor

@opatel99 opatel99 commented Feb 3, 2022

Background

During inserts, JdbcPageSink would not accept time and timestamp Presto types because the cases were missing from the case blocks. As a result, writes from MySQL and PostgreSQL of those types were incompatible, while reads were allowed.

Issues

#13880
#7747

Test

MySQL (Local)

mysql> DROP TABLE test_date_and_time;
presto> CREATE TABLE mysql.testdb.test_date_and_time(col1 date, col2 time, col3 timestamp);
CREATE TABLE
presto> INSERT INTO mysql.testdb.test_date_and_time VALUES (date '2022-02-03', time '11:15:30', timestamp '2022-02-03 11:15');
INSERT: 1 row
presto> SELECT * from mysql.testdb.test_date_and_time;
    col1    |     col2     |          col3
------------+--------------+-------------------------
 2022-02-03 | 11:15:30.000 | 2022-02-03 11:15:00.000

PostgreSQL (Local)

test=# DROP TABLE test.test_date_and_time;
presto> CREATE TABLE postgresql.test.test_date_and_time(col1 date, col2 time, col3 timestamp);
CREATE TABLE
presto> INSERT INTO postgresql.test.test_date_and_time VALUES (date '2022-02-03', time '11:15:30', timestamp '2022-02-03 11:15');
INSERT: 1 row
presto> SELECT * from postgresql.test.test_date_and_time;
    col1    |     col2     |          col3
------------+--------------+-------------------------
 2022-02-03 | 11:15:30.000 | 2022-02-03 11:15:00.000

Unit Tests

com.facebook.presto:presto-base-jdbc
> testDateAndTimeInserts
> testCreateWithDateAndTimeColumns
> testCreateWithNullableColumns
[INFO] Tests run: 66, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 31.735 s - in TestSuite

Release Notes

== RELEASE NOTES ==

General Changes

  • Add time and timestamp inserts for Jdbc connectors

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 3, 2022

CLA Signed

The committers are authorized under a signed CLA.

  • ✅ Ohm Patel (436a9e45db89566328716ddd24d1a0937d3d3633)

Comment on lines 170 to 172
else if (TIME.equals(type)) {
statement.setTime(parameter, new Time(type.getLong(block, position)));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on my read of the code - time is stored in terms of UTC time and not local time and hence we may have to convert this to UTC time before writing the value back.

https://github.com/prestodb/presto/blob/master/presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/StandardReadMappings.java#L152 -> this is how we read the time value

https://github.com/prestodb/presto/blob/master/presto-common/src/main/java/com/facebook/presto/common/type/SqlTime.java#L98

@highker - what is your opinion here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's better to be consistent with DATE branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants