Skip to content

Fix failure of insertion of timestamp on JDBC #8201

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.

Sign up for GitHub

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

Closed
ShanBai6 wants to merge 1 commit into from
Closed

Fix failure of insertion of timestamp on JDBC #8201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcRecordSink.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,15 @@
import java.sql.PreparedStatement;
import java.sql.SQLException;
import java.sql.SQLNonTransientException;
import java.sql.Timestamp;
import java.util.Collection;
import java.util.List;
import java.util.concurrent.TimeUnit;

import static com.facebook.presto.plugin.jdbc.JdbcErrorCode.JDBC_ERROR;
import static com.facebook.presto.plugin.jdbc.JdbcErrorCode.JDBC_NON_TRANSIENT_ERROR;
import static com.facebook.presto.spi.type.DateType.DATE;
import static com.facebook.presto.spi.type.TimestampType.TIMESTAMP;
import static com.google.common.base.Preconditions.checkState;
import static java.nio.charset.StandardCharsets.UTF_8;

Expand Down Expand Up @@ -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);
Copy link
Copy Markdown
Member

@arhimondr arhimondr Jun 6, 2017

Choose a reason for hiding this comment

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

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; }
    },

Copy link
Copy Markdown
Contributor

@findepi findepi Jun 7, 2017

Choose a reason for hiding this comment

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

Has this been addressed?

Copy link
Copy Markdown
Author

@ShanBai6 ShanBai6 Jun 7, 2017

Choose a reason for hiding this comment

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

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

long localMillis = ISOChronology.getInstanceUTC().getZone().getMillisKeepLocal(DateTimeZone.getDefault(), utcMillis);
statement.setTimestamp(next(), new Timestamp(localMillis));
Copy link
Copy Markdown
Member

@arhimondr arhimondr Jun 6, 2017

Choose a reason for hiding this comment

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

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);)

Copy link
Copy Markdown
Contributor

@findepi findepi Jun 6, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

@ShanBai6 ShanBai6 Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

}
else {
statement.setLong(next(), value);
}
Expand Down
50 changes: 34 additions & 16 deletions ...o-mysql/src/test/java/com/facebook/presto/plugin/mysql/TestMySqlIntegrationSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package com.facebook.presto.plugin.mysql;

import com.facebook.presto.Session;
import com.facebook.presto.spi.type.VarcharType;
import com.facebook.presto.testing.MaterializedResult;
import com.facebook.presto.testing.MaterializedRow;
import com.facebook.presto.tests.AbstractTestIntegrationSmokeTest;
Expand All @@ -25,8 +26,11 @@
import java.sql.DriverManager;
import java.sql.SQLException;
import java.sql.Statement;
import java.sql.Timestamp;

import static com.facebook.presto.plugin.mysql.MySqlQueryRunner.createMySqlQueryRunner;
import static com.facebook.presto.spi.type.BigintType.BIGINT;
import static com.facebook.presto.spi.type.TimestampType.TIMESTAMP;
import static com.facebook.presto.spi.type.VarcharType.VARCHAR;
import static com.facebook.presto.testing.TestingSession.testSessionBuilder;
import static com.facebook.presto.testing.assertions.Assert.assertEquals;
Expand Down Expand Up @@ -64,31 +68,45 @@ public final void destroy()
public void testDescribeTable()
throws Exception
{
// we need specific implementation of this tests due to specific Presto<->Mysql varchar length mapping.
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_describe_table (c_bigint bigint, c_varchar_255 varchar(255), c_varchar_15 varchar(15), c_double double, c_date date, c_integer integer, c_timestamp timestamp)");
MaterializedResult actualColumns = computeActual("DESC test_describe_table").toJdbcTypes();
MaterializedResult expectedColumns = MaterializedResult.resultBuilder(getQueryRunner().getDefaultSession(), VARCHAR, VARCHAR, VARCHAR, VARCHAR)
.row("orderkey", "bigint", "", "")
.row("custkey", "bigint", "", "")
.row("orderstatus", "varchar(255)", "", "")
.row("totalprice", "double", "", "")
.row("orderdate", "date", "", "")
.row("orderpriority", "varchar(255)", "", "")
.row("clerk", "varchar(255)", "", "")
.row("shippriority", "integer", "", "")
.row("comment", "varchar(255)", "", "")
.row("c_bigint", "bigint", "", "")
.row("c_varchar_255", "varchar(255)", "", "")
.row("c_varchar_15", "varchar(15)", "", "")
.row("c_double", "double", "", "")
.row("c_date", "date", "", "")
.row("c_integer", "integer", "", "")
.row("c_timestamp", "timestamp", "", "")
.build();
assertEquals(actualColumns, expectedColumns);
assertUpdate("DROP TABLE test_describe_table");
}

@Test
public void testCreateTableFromSelect()
throws Exception
{
execute("CREATE TABLE tpch.temp_table_with_timestamp_column (c_timestamp timestamp)");
assertUpdate(getQueryRunner().getDefaultSession(), "INSERT INTO temp_table_with_timestamp_column VALUES (date_parse('1970-12-01 00:00:01','%Y-%m-%d %H:%i:%s'))", 1);
assertUpdate(getQueryRunner().getDefaultSession(), "CREATE TABLE test_create_table_as_select AS SELECT * FROM temp_table_with_timestamp_column", 1);
assertUpdate("DROP TABLE temp_table_with_timestamp_column");
assertUpdate("DROP TABLE test_create_table_as_select");
}

@Test
public void testInsert()
throws Exception
{
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 (c_bigint bigint, c_varchar_100 varchar(100), c_timestamp timestamp)");
assertUpdate(getQueryRunner().getDefaultSession(), "INSERT INTO test_insert VALUES (123, 'test', date_parse('1970-12-01 00:00:01','%Y-%m-%d %H:%i:%s'))", 1);
assertUpdate(getQueryRunner().getDefaultSession(), "INSERT INTO test_insert VALUES (123, 'test', date_parse('2030-01-01 23:59:59','%Y-%m-%d %H:%i:%s'))", 1);
MaterializedResult actual = computeActual("SELECT * FROM test_insert");
MaterializedResult expected = MaterializedResult.resultBuilder(getSession(), BIGINT, VarcharType.createVarcharType(100), TIMESTAMP)
.row((long) 123, "test", Timestamp.valueOf("1970-12-01 00:00:01"))
.row((long) 123, "test", Timestamp.valueOf("2030-01-01 23:59:59"))
.build();
assertEquals(actual, expected);
assertUpdate("DROP TABLE test_insert");
}

Expand Down
43 changes: 40 additions & 3 deletions ...c/test/java/com/facebook/presto/plugin/postgresql/TestPostgreSqlIntegrationSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package com.facebook.presto.plugin.postgresql;

import com.facebook.presto.testing.MaterializedResult;
import com.facebook.presto.tests.AbstractTestIntegrationSmokeTest;
import io.airlift.testing.postgresql.TestingPostgreSqlServer;
import org.testng.annotations.AfterClass;
Expand All @@ -23,7 +24,12 @@
import java.sql.DriverManager;
import java.sql.SQLException;
import java.sql.Statement;
import java.sql.Timestamp;

import static com.facebook.presto.spi.type.BigintType.BIGINT;
import static com.facebook.presto.spi.type.TimestampType.TIMESTAMP;
import static com.facebook.presto.spi.type.VarcharType.VARCHAR;
import static com.facebook.presto.testing.assertions.Assert.assertEquals;
import static io.airlift.tpch.TpchTable.ORDERS;
import static org.testng.Assert.assertTrue;

Expand Down Expand Up @@ -53,13 +59,44 @@ public final void destroy()
postgreSqlServer.close();
}

@Test
public void testDescribeTable()
throws Exception
{
execute("CREATE TABLE tpch.test_decribe_table (c_timestamp timestamp without time zone, c_timestamp_tz timestamp with time zone)");
MaterializedResult actualColumns = computeActual(getQueryRunner().getDefaultSession(), "DESC test_decribe_table").toJdbcTypes();
MaterializedResult expectedColumns = MaterializedResult.resultBuilder(getQueryRunner().getDefaultSession(), VARCHAR, VARCHAR, VARCHAR, VARCHAR)
.row("c_timestamp", "timestamp", "", "")
.row("c_timestamp_tz", "timestamp", "", "")
.build();
assertEquals(actualColumns, expectedColumns);
assertUpdate("DROP TABLE test_decribe_table");
}

@Test
public void testCreateTableFromSelect()
throws Exception
{
execute("CREATE TABLE tpch.temp_table_with_timestamp_column (c_timestamp timestamp)");
assertUpdate(getQueryRunner().getDefaultSession(), "INSERT INTO temp_table_with_timestamp_column VALUES (date_parse('1970-12-01 00:00:01','%Y-%m-%d %H:%i:%s'))", 1);
assertUpdate(getQueryRunner().getDefaultSession(), "CREATE TABLE test_create_table_as_select AS SELECT * FROM temp_table_with_timestamp_column", 1);
assertUpdate("DROP TABLE temp_table_with_timestamp_column");
assertUpdate("DROP TABLE test_create_table_as_select");
}

@Test
public void testInsert()
throws Exception
{
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 (c_bigint bigint, c_varchar varchar, c_timestamp timestamp)");
assertUpdate("INSERT INTO test_insert VALUES (123, 'test', date_parse('1970-12-01 00:00:01','%Y-%m-%d %H:%i:%s'))", 1);
assertUpdate("INSERT INTO test_insert VALUES (123, 'test', date_parse('2030-01-01 23:59:59','%Y-%m-%d %H:%i:%s'))", 1);
MaterializedResult actual = computeActual("SELECT * FROM test_insert");
MaterializedResult expected = MaterializedResult.resultBuilder(getSession(), BIGINT, VARCHAR, TIMESTAMP)
.row((long) 123, "test", Timestamp.valueOf("1970-12-01 00:00:01"))
.row((long) 123, "test", Timestamp.valueOf("2030-01-01 23:59:59"))
.build();
assertEquals(actual, expected);
assertUpdate("DROP TABLE test_insert");
}

Expand Down