Fix failure of insertion of timestamp on JDBC #8201

ShanBai6 · 2017-06-06T17:01:38Z

Fix the timestamp issue #7747 and Add/Extend original test to cover the timestamp type as well.

facebook-github-bot · 2017-06-06T17:01:42Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

arhimondr · 2017-06-06T17:05:06Z

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

@@ -130,6 +132,11 @@ public void appendLong(long value)
                long localMillis = ISOChronology.getInstanceUTC().getZone().getMillisKeepLocal(DateTimeZone.getDefault(), utcMillis);
                statement.setDate(next(), new Date(localMillis));
            }
+            else if (TIMESTAMP.equals(columnTypes.get(field))) {
+                long utcMillis = TimeUnit.MILLISECONDS.toMillis(value);


It does nothing

/** * Time unit representing one thousandth of a second */ MILLISECONDS { public long toNanos(long d) { return x(d, C2/C0, MAX/(C2/C0)); } public long toMicros(long d) { return x(d, C2/C1, MAX/(C2/C1)); } public long toMillis(long d) { return d; } public long toSeconds(long d) { return d/(C3/C2); } public long toMinutes(long d) { return d/(C4/C2); } public long toHours(long d) { return d/(C5/C2); } public long toDays(long d) { return d/(C6/C2); } public long convert(long d, TimeUnit u) { return u.toMillis(d); } int excessNanos(long d, long m) { return 0; } },

Has this been addressed?

This is the same portion of code as below, we decide to keep it as this for now.

arhimondr · 2017-06-06T17:23:54Z

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

+            else if (TIMESTAMP.equals(columnTypes.get(field))) {
+                long utcMillis = TimeUnit.MILLISECONDS.toMillis(value);
+                long localMillis = ISOChronology.getInstanceUTC().getZone().getMillisKeepLocal(DateTimeZone.getDefault(), utcMillis);
+                statement.setTimestamp(next(), new Timestamp(localMillis));


try to use the following method

statement.setTimestamp(next(), new Timestamp(utcMillis), Calendar.getInstance(/*force time zone to be UTC*/TimeZone.getTimeZone("UTC"), /*force calendar to be Gregorian*/ ENGLISH);)

AFAIK, postgres doesn't follow standard with respect to timestamp semantics. Did you check that?
In anyway, make sure value read from postgres is the same as value written earlier.

This will be done in a separate patch after further discussion.

arhimondr · 2017-06-06T17:27:04Z

presto-mysql/src/test/java/com/facebook/presto/plugin/mysql/TestMySqlIntegrationSmokeTest.java

-        MaterializedResult actualColumns = computeActual("DESC ORDERS").toJdbcTypes();
-
-        // some connectors don't support dates, and some do not support parametrized varchars, so we check multiple options
+        execute("CREATE TABLE tpch.test (test_bigint bigint, test_varchar varchar(255), test_vardiff varchar(15), test_double double, test_date date, test_integer integer, test_timestamp timestamp)");


Name test is too generic. Call it test_describe_table_all_types.

arhimondr · 2017-06-06T17:28:34Z

presto-mysql/src/test/java/com/facebook/presto/plugin/mysql/TestMySqlIntegrationSmokeTest.java

-        MaterializedResult actualColumns = computeActual("DESC ORDERS").toJdbcTypes();
-
-        // some connectors don't support dates, and some do not support parametrized varchars, so we check multiple options
+        execute("CREATE TABLE tpch.test (test_bigint bigint, test_varchar varchar(255), test_vardiff varchar(15), test_double double, test_date date, test_integer integer, test_timestamp timestamp)");


Prefix column names with c_ instead of test_. For instance c_varchar_255, c_varchar_15, c_timestamp, etc.

arhimondr · 2017-06-06T17:29:15Z

presto-mysql/src/test/java/com/facebook/presto/plugin/mysql/TestMySqlIntegrationSmokeTest.java

-        execute("CREATE TABLE tpch.test_insert (x bigint, y varchar(100))");
-        assertUpdate("INSERT INTO test_insert VALUES (123, 'test')", 1);
-        assertQuery("SELECT * FROM test_insert", "SELECT 123 x, 'test' y");
+        execute("CREATE TABLE tpch.test_insert (x bigint, y varchar(100), t_ts timestamp)");


Do "all types" test as it is done for describe statement.

Describe test could be done for all types, while the insertion has to wait for later as some data types still have issues.

arhimondr · 2017-06-06T17:29:33Z

presto-mysql/src/test/java/com/facebook/presto/plugin/mysql/TestMySqlIntegrationSmokeTest.java

-        execute("CREATE TABLE tpch.test_insert (x bigint, y varchar(100))");
-        assertUpdate("INSERT INTO test_insert VALUES (123, 'test')", 1);
-        assertQuery("SELECT * FROM test_insert", "SELECT 123 x, 'test' y");
+        execute("CREATE TABLE tpch.test_insert (x bigint, y varchar(100), t_ts timestamp)");


Same comment about column names

arhimondr · 2017-06-06T17:30:11Z

.../src/test/java/com/facebook/presto/plugin/postgresql/TestPostgreSqlIntegrationSmokeTest.java

+    public void testDescribeTable()
+            throws Exception
+    {
+        execute("CREATE TABLE tpch.test (test_timestamp timestamp without time zone, test_timestamp_tz timestamp with time zone)");


arhimondr · 2017-06-06T17:30:19Z

.../src/test/java/com/facebook/presto/plugin/postgresql/TestPostgreSqlIntegrationSmokeTest.java

-        execute("CREATE TABLE tpch.test_insert (x bigint, y varchar(100))");
-        assertUpdate("INSERT INTO test_insert VALUES (123, 'test')", 1);
-        assertQuery("SELECT * FROM test_insert", "SELECT 123 x, 'test' y");
+        execute("CREATE TABLE tpch.test_insert (x bigint, y varchar, t_ts timestamp)");


findepi · 2017-06-06T17:29:57Z

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

+            else if (TIMESTAMP.equals(columnTypes.get(field))) {
+                long utcMillis = TimeUnit.MILLISECONDS.toMillis(value);
+                long localMillis = ISOChronology.getInstanceUTC().getZone().getMillisKeepLocal(DateTimeZone.getDefault(), utcMillis);
+                statement.setTimestamp(next(), new Timestamp(localMillis));


AFAIK, postgres doesn't follow standard with respect to timestamp semantics. Did you check that?
In anyway, make sure value read from postgres is the same as value written earlier.

findepi · 2017-06-06T17:31:14Z

.../src/test/java/com/facebook/presto/plugin/postgresql/TestPostgreSqlIntegrationSmokeTest.java

-        execute("CREATE TABLE tpch.test_insert (x bigint, y varchar(100))");
-        assertUpdate("INSERT INTO test_insert VALUES (123, 'test')", 1);
-        assertQuery("SELECT * FROM test_insert", "SELECT 123 x, 'test' y");
+        execute("CREATE TABLE tpch.test_insert (x bigint, y varchar, t_ts timestamp)");


None of the tests exercises CTAS and #7747 mentions CTAS.

arhimondr · 2017-06-06T21:02:03Z

LGTM

kubum · 2019-01-09T10:48:47Z

Hi @arhimondr! It looks like an interesting PR, if I fix the conflicts would you be able to help and a look again into merging it? Thanks!

findepi · 2019-01-09T11:32:06Z

There is no correct and portable way for handling timestamps, especially when legacy_timestamp=false session toggle is set.
@kubum #12151 is a refactor that provides basis for handling the conversions in database-specific way where appropriate.

kubum · 2019-01-09T12:01:52Z

Thank you for the quick response @findepi!

maciejgrzybek assigned arhimondr Jun 6, 2017

maciejgrzybek requested a review from arhimondr June 6, 2017 17:03

arhimondr reviewed Jun 6, 2017

View reviewed changes

findepi reviewed Jun 6, 2017

View reviewed changes

Fix failure of insertion of timestamp on JDBC

f2e0faa

findepi closed this Jan 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix failure of insertion of timestamp on JDBC #8201

Fix failure of insertion of timestamp on JDBC #8201

ShanBai6 commented Jun 6, 2017

facebook-github-bot commented Jun 6, 2017

arhimondr Jun 6, 2017

findepi Jun 7, 2017

ShanBai6 Jun 7, 2017

arhimondr Jun 6, 2017

findepi Jun 6, 2017

ShanBai6 Jun 6, 2017 •

edited

Loading

arhimondr Jun 6, 2017

arhimondr Jun 6, 2017

arhimondr Jun 6, 2017

ShanBai6 Jun 6, 2017

arhimondr Jun 6, 2017

arhimondr Jun 6, 2017

arhimondr Jun 6, 2017

findepi Jun 6, 2017

findepi Jun 6, 2017

arhimondr commented Jun 6, 2017

kubum commented Jan 9, 2019

findepi commented Jan 9, 2019

kubum commented Jan 9, 2019

Fix failure of insertion of timestamp on JDBC #8201

Fix failure of insertion of timestamp on JDBC #8201

Conversation

ShanBai6 commented Jun 6, 2017

facebook-github-bot commented Jun 6, 2017

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

Choose a reason for hiding this comment

ShanBai6 Jun 6, 2017 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arhimondr commented Jun 6, 2017

kubum commented Jan 9, 2019

findepi commented Jan 9, 2019

kubum commented Jan 9, 2019

ShanBai6 Jun 6, 2017 •

edited

Loading