ClickHouse / clickhouse-java

ClickHouse Java Clients & JDBC Driver
https://clickhouse.com
Apache License 2.0
1.45k stars 537 forks source link

Timestamp read from database is treated as if it was in JVM timezone #1048

Open piotrp opened 2 years ago

piotrp commented 2 years ago

When reading a DateTime value I get a timestamp that is shifted back by my JVM timezone offset.

Server is in UTC, JVM in Europe/Warsaw (UTC+2). When I try to read a DateTime value 2021-08-13T11:00:00Z I get 2021-08-13T09:00:00Z.

Is this driver bug or am I doing something wrong?

I'm using clickhouse-jdbc 0.3.2-patch11 (revision: 27f8951). ClickHouse 22.6.3.35.

package test;

import com.clickhouse.jdbc.ClickHouseConnection;
import com.clickhouse.jdbc.ClickHouseDataSource;
import com.clickhouse.jdbc.ClickHouseDriver;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

import java.sql.Connection;
import java.sql.SQLException;
import java.time.Instant;
import java.util.Optional;
import java.util.TimeZone;

public class TimeTests {
    private final static String URL = "jdbc:clickhouse://localhost:18123/";
    private final static String USERNAME = "default";
    private final static String PASSWORD = "";

    static {
        System.out.println(ClickHouseDriver.class.getPackage().getImplementationVersion());
        TimeZone.setDefault(TimeZone.getTimeZone("Europe/Warsaw"));
    }

    @Test
    void testReading() throws SQLException {
        var dataSource = new ClickHouseDataSource(URL);
        try (var conn = dataSource.getConnection(USERNAME, PASSWORD)) {
            prepareTestTable(conn);

            execSql("INSERT INTO dates(d) VALUES (toDateTime(toUnixTimestamp('2021-08-13 11:00:00', 'UTC'), 'UTC'))", conn);

            try (var stmt = conn.createStatement()) {
                var rs = stmt.executeQuery("SELECT formatDateTime(d, '%F %T'), toUnixTimestamp(d), d FROM dates");
                Assertions.assertTrue(rs.next());
                Assertions.assertEquals("2021-08-13 11:00:00", rs.getString(1));
                Assertions.assertEquals(Instant.parse("2021-08-13T11:00:00Z"), Instant.ofEpochSecond(rs.getLong(2)));
                var ts = rs.getTimestamp(3);
                // Assertion below fails with:
                // Expected :2021-08-13T11:00:00Z
                // Actual   :2021-08-13T09:00:00Z
                // looks like UTC date from server was treated as if it was in JVM TZ (Europe/Warsaw = UTC+2)
                Assertions.assertEquals(Instant.parse("2021-08-13T11:00:00Z"), ts.toInstant());
            }
        }
    }

    void prepareTestTable(ClickHouseConnection conn) throws SQLException {
        Assertions.assertEquals(TimeZone.getTimeZone("UTC"), conn.getServerTimeZone());
        Assertions.assertEquals(TimeZone.getTimeZone("Europe/Warsaw"), conn.getJvmTimeZone());
        Assertions.assertEquals(Optional.empty(), conn.getEffectiveTimeZone());

        execSql("DROP TABLE IF EXISTS dates", conn);
        execSql("CREATE TABLE dates (d DateTime) ENGINE=Memory", conn);
    }

    void execSql(String sql, Connection conn) throws SQLException {
        try (var stmt = conn.createStatement()) {
            stmt.execute(sql);
        }
    }

}
piotrp commented 2 years ago

I believe the issue is in the way getTimestamp works. Value is read correctly from database, as 2021-08-13 11:00:00 at UTC: obraz

but here:

https://github.com/ClickHouse/clickhouse-jdbc/blob/27f8951cdc380bf1799f76efc21fc377fc6d5981/clickhouse-jdbc/src/main/java/com/clickhouse/jdbc/ClickHouseResultSet.java#L626-L634

it's read into dt as LocalDateTime(2021-08-13T11:00) (tz is null here, but both possible code paths give the same result). Then it's copied into defaultCalendar which is at JVM's time zone.

zhicwu commented 2 years ago

Hi @piotrp, sorry for the late response. The behavior is same as MySQL and legacy driver, but I agree that use_server_time_zone option is really confusing. Perhaps you can use DateTime column with timezone argument or use_time_zone option as shown in below example?

https://github.com/ClickHouse/clickhouse-jdbc/blob/f285b73d45597e29cf36dff4daf4cb4f9f53453a/clickhouse-jdbc/src/test/java/com/clickhouse/jdbc/ClickHouseStatementTest.java#L578-L593

piotrp commented 2 years ago

No, the legacy driver worked correctly. I stumbled upon this issue when I tried upgrading and my JDBC code using getTimestamp failed.

I created a test case at https://github.com/piotrp/clickhouse-driver-issue-1048, where you can see that the same test passes when run with legacy driver, and fails with the new one. I also added a test demonstrating that round trip with setTimestamp/getTimesatamp doesn't work correctly. This should be runnable under Java 17 by:

docker-compose up -d
mvn --fail-at-end test

Thanks for this pointer, for now I worked around this by using getObject(.., Instant.class). Fortunately I'm using hand-crafted row mappers, so it was easy to do.

zhicwu commented 2 years ago

Thanks for sharing the test case. Legacy driver does not support timezone very well mainly due to the text-based data format. It's been enhanced in the new driver and this test case was added to ensure the driver works in the same way as MySQL.

It's recommended to use getObject instead of getTimestamp/getDate not only because it turns LocalDate/LocalDateTime/OffsetDateTime over legacy Date/Timestamp, but also ~10x better performance.

Let me see if I can fix this together with #955.