Skip to content

Commit 5efa463

Browse files
tdcmeehantdcmeehan
authored
fix(plugin-postgres): Fix Timestamp values in JDBC connectors (#26449)
1 parent c1491ef commit 5efa463

File tree

7 files changed

+350
-23
lines changed

7 files changed

+350
-23
lines changed

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,8 @@
7474
import static com.facebook.presto.plugin.jdbc.JdbcWarningCode.USE_OF_DEPRECATED_CONFIGURATION_PROPERTY;
7575
import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.jdbcTypeToReadMapping;
7676
import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.prestoTypeToWriteMapping;
77+
import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.timestampReadMapping;
78+
import static com.facebook.presto.plugin.jdbc.mapping.StandardColumnMappings.timestampReadMappingLegacy;
7779
import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND;
7880
import static com.facebook.presto.spi.StandardErrorCode.NOT_SUPPORTED;
7981
import static com.google.common.base.MoreObjects.firstNonNull;
@@ -276,6 +278,10 @@ public List<JdbcColumnHandle> getColumns(ConnectorSession session, JdbcTableHand
276278
@Override
277279
public Optional<ReadMapping> toPrestoType(ConnectorSession session, JdbcTypeHandle typeHandle)
278280
{
281+
if (typeHandle.getJdbcType() == java.sql.Types.TIMESTAMP) {
282+
boolean legacyTimestamp = session.getSqlFunctionProperties().isLegacyTimestamp();
283+
return Optional.of(legacyTimestamp ? timestampReadMappingLegacy() : timestampReadMapping());
284+
}
279285
return jdbcTypeToReadMapping(typeHandle);
280286
}
281287

@@ -794,9 +800,9 @@ protected String toSqlType(Type type)
794800
throw new PrestoException(NOT_SUPPORTED, "Unsupported column type: " + type.getDisplayName());
795801
}
796802

797-
public WriteMapping toWriteMapping(Type type)
803+
public WriteMapping toWriteMapping(ConnectorSession session, Type type)
798804
{
799-
Optional<WriteMapping> writeMapping = prestoTypeToWriteMapping(type);
805+
Optional<WriteMapping> writeMapping = prestoTypeToWriteMapping(session, type);
800806
if (writeMapping.isPresent()) {
801807
return writeMapping.get();
802808
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ default boolean schemaExists(ConnectorSession session, JdbcIdentity identity, St
5353

5454
Optional<ReadMapping> toPrestoType(ConnectorSession session, JdbcTypeHandle typeHandle);
5555

56-
WriteMapping toWriteMapping(Type type);
56+
WriteMapping toWriteMapping(ConnectorSession session, Type type);
5757

5858
ConnectorSplitSource getSplits(ConnectorSession session, JdbcIdentity identity, JdbcTableLayoutHandle layoutHandle);
5959

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ public JdbcPageSink(ConnectorSession session, JdbcOutputTableHandle handle, Jdbc
7777

7878
columnTypes = handle.getColumnTypes();
7979
columnWriters = columnTypes.stream().map(type -> {
80-
WriteFunction writeFunction = jdbcClient.toWriteMapping(type).getWriteFunction();
80+
WriteFunction writeFunction = jdbcClient.toWriteMapping(session, type).getWriteFunction();
8181
verify(type.getJavaType() == writeFunction.getJavaType(),
8282
format("Presto type %s is not compatible with write function %s accepting %s", type, writeFunction, writeFunction.getJavaType()));
8383
return writeFunction;

‎presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/mapping/StandardColumnMappings.java‎

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import com.facebook.presto.common.type.UuidType;
2323
import com.facebook.presto.common.type.VarcharType;
2424
import com.facebook.presto.plugin.jdbc.JdbcTypeHandle;
25+
import com.facebook.presto.spi.ConnectorSession;
2526
import com.google.common.base.CharMatcher;
2627
import com.google.common.primitives.Shorts;
2728
import com.google.common.primitives.SignedBytes;
@@ -38,7 +39,9 @@
3839
import java.sql.Timestamp;
3940
import java.sql.Types;
4041
import java.time.Instant;
42+
import java.util.Calendar;
4143
import java.util.Optional;
44+
import java.util.TimeZone;
4245

4346
import static com.facebook.presto.common.type.BigintType.BIGINT;
4447
import static com.facebook.presto.common.type.BooleanType.BOOLEAN;
@@ -91,6 +94,7 @@ public final class StandardColumnMappings
9194
private StandardColumnMappings() {}
9295

9396
private static final ISOChronology UTC_CHRONOLOGY = ISOChronology.getInstanceUTC();
97+
private static final Calendar UTC_CALENDAR = Calendar.getInstance(TimeZone.getTimeZone("UTC"));
9498

9599
public static ReadMapping booleanReadMapping()
96100
{
@@ -260,22 +264,37 @@ public static WriteMapping timeWriteMapping()
260264
public static ReadMapping timestampReadMapping()
261265
{
262266
return createLongReadMapping(TIMESTAMP, (resultSet, columnIndex) -> {
263-
/*
264-
* TODO `resultSet.getTimestamp(columnIndex)` returns wrong value if JVM's zone had forward offset change and the local time
265-
* corresponding to timestamp value being retrieved was not present (a 'gap'), this includes regular DST changes (e.g. Europe/Warsaw)
266-
* and one-time policy changes (Asia/Kathmandu's shift by 15 minutes on January 1, 1986, 00:00:00).
267-
* The problem can be averted by using `resultSet.getObject(columnIndex, LocalDateTime.class)` -- but this is not universally supported by JDBC drivers.
268-
*/
267+
Timestamp timestamp = resultSet.getTimestamp(columnIndex, UTC_CALENDAR);
268+
return timestamp.getTime();
269+
});
270+
}
271+
272+
@Deprecated
273+
public static ReadMapping timestampReadMappingLegacy()
274+
{
275+
return createLongReadMapping(TIMESTAMP, (resultSet, columnIndex) -> {
269276
Timestamp timestamp = resultSet.getTimestamp(columnIndex);
270277
return timestamp.getTime();
271278
});
272279
}
273280

274281
public static WriteMapping timestampWriteMapping(TimestampType timestampType)
275282
{
276-
return createLongWriteMapping((statement, index, value) -> statement.setTimestamp(index, Timestamp.from(Instant.ofEpochSecond(
277-
timestampType.getEpochSecond(value),
278-
timestampType.getNanos(value)))));
283+
return createLongWriteMapping((statement, index, value) -> {
284+
statement.setTimestamp(index, Timestamp.from(Instant.ofEpochSecond(
285+
timestampType.getEpochSecond(value),
286+
timestampType.getNanos(value))), UTC_CALENDAR);
287+
});
288+
}
289+
290+
@Deprecated
291+
public static WriteMapping timestampWriteMappingLegacy(TimestampType timestampType)
292+
{
293+
return createLongWriteMapping((statement, index, value) -> {
294+
statement.setTimestamp(index, Timestamp.from(Instant.ofEpochSecond(
295+
timestampType.getEpochSecond(value),
296+
timestampType.getNanos(value))));
297+
});
279298
}
280299
public static WriteMapping uuidWriteMapping()
281300
{
@@ -358,7 +377,7 @@ public static Optional<ReadMapping> jdbcTypeToReadMapping(JdbcTypeHandle type)
358377
return Optional.empty();
359378
}
360379

361-
public static Optional<WriteMapping> prestoTypeToWriteMapping(Type type)
380+
public static Optional<WriteMapping> prestoTypeToWriteMapping(ConnectorSession session, Type type)
362381
{
363382
if (type.equals(BOOLEAN)) {
364383
return Optional.of(booleanWriteMapping());
@@ -394,7 +413,8 @@ else if (type instanceof DateType) {
394413
return Optional.of(dateWriteMapping());
395414
}
396415
else if (type instanceof TimestampType) {
397-
return Optional.of(timestampWriteMapping((TimestampType) type));
416+
boolean legacyTimestamp = session.getSqlFunctionProperties().isLegacyTimestamp();
417+
return Optional.of(legacyTimestamp ? timestampWriteMappingLegacy((TimestampType) type) : timestampWriteMapping((TimestampType) type));
398418
}
399419
else if (type.equals(TIME)) {
400420
return Optional.of(timeWriteMapping());

‎presto-base-jdbc/src/test/java/com/facebook/presto/plugin/jdbc/TestJdbcQueryBuilder.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,12 @@
3737
import java.sql.Timestamp;
3838
import java.time.LocalDate;
3939
import java.time.LocalDateTime;
40+
import java.time.ZoneOffset;
41+
import java.util.Calendar;
4042
import java.util.List;
4143
import java.util.Locale;
4244
import java.util.Optional;
45+
import java.util.TimeZone;
4346

4447
import static com.facebook.airlift.testing.Assertions.assertContains;
4548
import static com.facebook.presto.common.type.BigintType.BIGINT;
@@ -74,6 +77,7 @@
7477
@Test(singleThreaded = true)
7578
public class TestJdbcQueryBuilder
7679
{
80+
private static final Calendar UTC_CALENDAR = Calendar.getInstance(TimeZone.getTimeZone("UTC"));
7781
private TestingDatabase database;
7882
private JdbcClient jdbcClient;
7983
private ConnectorSession session;
@@ -348,7 +352,7 @@ public void testBuildSqlWithTimestamp()
348352
ResultSet resultSet = preparedStatement.executeQuery()) {
349353
ImmutableSet.Builder<Timestamp> builder = ImmutableSet.builder();
350354
while (resultSet.next()) {
351-
builder.add((Timestamp) resultSet.getObject("col_6"));
355+
builder.add(resultSet.getTimestamp("col_6", UTC_CALENDAR));
352356
}
353357
assertEquals(builder.build(), ImmutableSet.of(
354358
toTimestamp(2016, 6, 3, 0, 23, 37),
@@ -379,7 +383,7 @@ public void testEmptyBuildSql()
379383

380384
private static Timestamp toTimestamp(int year, int month, int day, int hour, int minute, int second)
381385
{
382-
return Timestamp.valueOf(LocalDateTime.of(year, month, day, hour, minute, second));
386+
return Timestamp.from(LocalDateTime.of(year, month, day, hour, minute, second).toInstant(ZoneOffset.UTC));
383387
}
384388

385389
private static long toDays(int year, int month, int day)

‎presto-mysql/src/test/java/com/facebook/presto/plugin/mysql/TestMySqlTypeMapping.java‎

Lines changed: 149 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,10 @@
3131
import org.testng.annotations.Test;
3232

3333
import java.math.BigDecimal;
34+
import java.sql.Connection;
35+
import java.sql.DriverManager;
36+
import java.sql.ResultSet;
37+
import java.sql.Statement;
3438
import java.time.LocalDate;
3539
import java.time.ZoneId;
3640

@@ -53,6 +57,8 @@
5357
import static com.google.common.base.Strings.repeat;
5458
import static com.google.common.base.Verify.verify;
5559
import static java.lang.String.format;
60+
import static org.testng.Assert.assertEquals;
61+
import static org.testng.Assert.assertTrue;
5662

5763
@Test
5864
public class TestMySqlTypeMapping
@@ -254,15 +260,154 @@ public void testDate()
254260
}
255261

256262
@Test
257-
public void testDatetime()
263+
public void testDatetimeUnderlyingStorageVerification()
264+
throws Exception
258265
{
259-
// TODO MySQL datetime is not correctly read (see comment in StandardColumnMappings.timestampReadMapping), but testing this is hard because of #7122
266+
String jdbcUrl = mysqlContainer.getJdbcUrl();
267+
String jdbcUrlWithCredentials = format("%s%suser=%s&password=%s",
268+
jdbcUrl,
269+
jdbcUrl.contains("?") ? "&" : "?",
270+
mysqlContainer.getUsername(),
271+
mysqlContainer.getPassword());
272+
JdbcSqlExecutor jdbcExecutor = new JdbcSqlExecutor(jdbcUrlWithCredentials);
273+
274+
try {
275+
jdbcExecutor.execute("CREATE TABLE tpch.test_datetime_storage (" +
276+
"id INT PRIMARY KEY, " +
277+
"dt DATETIME(6), " +
278+
"source VARCHAR(10))");
279+
280+
// MySQL insertion, MySQL retrieval, and Presto retrieval all agree on wall clock time
281+
jdbcExecutor.execute("INSERT INTO tpch.test_datetime_storage VALUES (1, '1970-01-01 00:00:00.000000', 'jdbc')");
282+
283+
try (Connection conn = DriverManager.getConnection(jdbcUrlWithCredentials);
284+
Statement stmt = conn.createStatement();
285+
ResultSet rs = stmt.executeQuery("SELECT CAST(dt AS CHAR) FROM tpch.test_datetime_storage WHERE id = 1")) {
286+
assertTrue(rs.next(), "Expected one row");
287+
String dbValue1 = rs.getString(1);
288+
assertEquals(dbValue1, "1970-01-01 00:00:00.000000", "JDBC insert should store wall clock time 1970-01-01 00:00:00 in DB");
289+
}
290+
291+
Session session = Session.builder(getQueryRunner().getDefaultSession())
292+
.setSystemProperty("legacy_timestamp", "false")
293+
.build();
294+
assertQuery(session,
295+
"SELECT dt FROM mysql.tpch.test_datetime_storage WHERE id = 1",
296+
"VALUES TIMESTAMP '1970-01-01 00:00:00.000000'");
297+
298+
// Presto insertion, retrieval via MySQL, and retrieval via Presto all agree on wall clock time
299+
assertUpdate(session, "INSERT INTO mysql.tpch.test_datetime_storage VALUES (2, TIMESTAMP '2023-06-15 14:30:00.000000', 'presto')", 1);
300+
301+
try (Connection conn = DriverManager.getConnection(jdbcUrlWithCredentials);
302+
Statement stmt = conn.createStatement();
303+
ResultSet rs = stmt.executeQuery("SELECT CAST(dt AS CHAR) FROM tpch.test_datetime_storage WHERE id = 2")) {
304+
assertTrue(rs.next(), "Expected one row");
305+
String dbValue2 = rs.getString(1);
306+
assertEquals(dbValue2, "2023-06-15 14:30:00.000000", "Presto insert should store wall clock time 2023-06-15 14:30:00 in DB");
307+
}
308+
309+
assertQuery(session,
310+
"SELECT dt FROM mysql.tpch.test_datetime_storage WHERE id = 2",
311+
"VALUES TIMESTAMP '2023-06-15 14:30:00.000000'");
312+
313+
for (String timeZoneId : ImmutableList.of("UTC", "America/New_York", "Asia/Tokyo", "Europe/Warsaw")) {
314+
Session sessionWithTimezone = Session.builder(getQueryRunner().getDefaultSession())
315+
.setTimeZoneKey(TimeZoneKey.getTimeZoneKey(timeZoneId))
316+
.setSystemProperty("legacy_timestamp", "false")
317+
.build();
318+
319+
assertQuery(sessionWithTimezone,
320+
"SELECT dt FROM mysql.tpch.test_datetime_storage WHERE id = 1",
321+
"VALUES TIMESTAMP '1970-01-01 00:00:00.000000'");
322+
323+
assertQuery(sessionWithTimezone,
324+
"SELECT dt FROM mysql.tpch.test_datetime_storage WHERE id = 2",
325+
"VALUES TIMESTAMP '2023-06-15 14:30:00.000000'");
326+
}
327+
}
328+
finally {
329+
jdbcExecutor.execute("DROP TABLE IF EXISTS tpch.test_datetime_storage");
330+
}
260331
}
261332

262333
@Test
263-
public void testTimestamp()
334+
public void testDatetimeLegacyUnderlyingStorageVerification()
335+
throws Exception
264336
{
265-
// TODO MySQL timestamp is not correctly read (see comment in StandardColumnMappings.timestampReadMapping), but testing this is hard because of #7122
337+
String jdbcUrl = mysqlContainer.getJdbcUrl();
338+
String jdbcUrlWithCredentials = format("%s%suser=%s&password=%s",
339+
jdbcUrl,
340+
jdbcUrl.contains("?") ? "&" : "?",
341+
mysqlContainer.getUsername(),
342+
mysqlContainer.getPassword());
343+
JdbcSqlExecutor jdbcExecutor = new JdbcSqlExecutor(jdbcUrlWithCredentials);
344+
345+
try {
346+
jdbcExecutor.execute("CREATE TABLE tpch.test_datetime_legacy_storage (" +
347+
"id INT PRIMARY KEY, " +
348+
"dt DATETIME(6), " +
349+
"source VARCHAR(10))");
350+
351+
// MySQL insertion and MySQL retrieval agree, Presto incorrectly interprets DB value due to legacy mode
352+
jdbcExecutor.execute("INSERT INTO tpch.test_datetime_legacy_storage VALUES (1, '1970-01-01 00:00:00.000000', 'jdbc')");
353+
354+
// Prove that the value is 1970-01-01 00:00:00 by reading directly from the DB via JDBC
355+
try (Connection conn = DriverManager.getConnection(jdbcUrlWithCredentials);
356+
Statement stmt = conn.createStatement();
357+
ResultSet rs = stmt.executeQuery("SELECT CAST(dt AS CHAR) FROM tpch.test_datetime_legacy_storage WHERE id = 1")) {
358+
assertTrue(rs.next(), "Expected one row");
359+
String dbValue1 = rs.getString(1);
360+
assertEquals(dbValue1, "1970-01-01 00:00:00.000000", "JDBC insert should store wall clock time 1970-01-01 00:00:00 in DB");
361+
}
362+
363+
// In legacy mode, DB value 1970-01-01 00:00:00 is interpreted as if it's in JVM timezone (America/Bahia_Banderas UTC-7)
364+
// and then converted to the session timezone. Since both are the same (America/Bahia_Banderas),
365+
// the offset comes from treating the wall-clock DB time as UTC, resulting in 1969-12-31 20:00:00
366+
Session legacySession = Session.builder(getQueryRunner().getDefaultSession())
367+
.setSystemProperty("legacy_timestamp", "true")
368+
.build();
369+
assertQuery(legacySession,
370+
"SELECT dt FROM mysql.tpch.test_datetime_legacy_storage WHERE id = 1",
371+
"VALUES TIMESTAMP '1969-12-31 20:00:00.000000'");
372+
373+
// Presto insertion with legacy mode, verify DB storage via JDBC (should apply JVM timezone conversion during write)
374+
assertUpdate(legacySession, "INSERT INTO mysql.tpch.test_datetime_legacy_storage VALUES (2, TIMESTAMP '2023-06-15 14:30:00.000000', 'presto')", 1);
375+
376+
try (Connection conn = DriverManager.getConnection(jdbcUrlWithCredentials);
377+
Statement stmt = conn.createStatement();
378+
ResultSet rs = stmt.executeQuery("SELECT CAST(dt AS CHAR) FROM tpch.test_datetime_legacy_storage WHERE id = 2")) {
379+
assertTrue(rs.next(), "Expected one row");
380+
String dbValue2 = rs.getString(1);
381+
// JVM timezone is America/Bahia_Banderas (UTC-7), so 2023-06-15 14:30:00 becomes 2023-06-14 19:30:00.000000
382+
assertEquals(dbValue2, "2023-06-14 19:30:00.000000", "Legacy mode applies timezone conversion during write, expected 2023-06-14 19:30:00.000000");
383+
}
384+
385+
// Verify Presto reads it back correctly in legacy mode (round-trip should work)
386+
assertQuery(legacySession,
387+
"SELECT dt FROM mysql.tpch.test_datetime_legacy_storage WHERE id = 2",
388+
"VALUES TIMESTAMP '2023-06-15 14:30:00.000000'");
389+
390+
// DB value 1970-01-01 00:00:00 is interpreted as JVM timezone (America/Bahia_Banderas UTC-7),
391+
// then converted to the session timezone
392+
Session legacyUtcSession = Session.builder(getQueryRunner().getDefaultSession())
393+
.setTimeZoneKey(TimeZoneKey.getTimeZoneKey("UTC"))
394+
.setSystemProperty("legacy_timestamp", "true")
395+
.build();
396+
assertQuery(legacyUtcSession,
397+
"SELECT dt FROM mysql.tpch.test_datetime_legacy_storage WHERE id = 1",
398+
"VALUES TIMESTAMP '1970-01-01 07:00:00.000000'");
399+
400+
Session legacyTokyoSession = Session.builder(getQueryRunner().getDefaultSession())
401+
.setTimeZoneKey(TimeZoneKey.getTimeZoneKey("Asia/Tokyo"))
402+
.setSystemProperty("legacy_timestamp", "true")
403+
.build();
404+
assertQuery(legacyTokyoSession,
405+
"SELECT dt FROM mysql.tpch.test_datetime_legacy_storage WHERE id = 1",
406+
"VALUES TIMESTAMP '1970-01-01 16:00:00.000000'");
407+
}
408+
finally {
409+
jdbcExecutor.execute("DROP TABLE IF EXISTS tpch.test_datetime_legacy_storage");
410+
}
266411
}
267412

268413
private void testUnsupportedDataType(String databaseDataType)

0 commit comments

Comments
 (0)