This is an automated email from the ASF dual-hosted git repository. amanin pushed a commit to branch fix/sql-temporal in repository https://gitbox.apache.org/repos/asf/sis.git
commit 95cc840866a828f4a69731844fd6fd2632f8b03b Author: Alexis Manin <[email protected]> AuthorDate: Tue May 24 10:52:26 2022 +0200 fix(Storage): remove incorrect timezone shift on sql time conversion The previous code assumed that return java.sql.Time object would math both time and timezone specified at insertion. This is not the case. In fact, tested databases engines return a time from start of day UTC equal to inserted time. The "timezone" of returned java.sql.Time object is totally unrelated, and apparently set to database/system timezone. This is very confusing. --- .../sis/internal/sql/feature/ValueGetter.java | 25 ++++++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/ValueGetter.java b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/ValueGetter.java index 97807d0f79..4e1fff09b0 100644 --- a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/ValueGetter.java +++ b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/ValueGetter.java @@ -18,7 +18,6 @@ package org.apache.sis.internal.sql.feature; import java.time.LocalDate; import java.time.LocalDateTime; -import java.util.Calendar; import java.util.Collection; import java.sql.Array; import java.sql.ResultSet; @@ -306,7 +305,10 @@ public abstract class ValueGetter<T> { * This getter delegates to {@link ResultSet#getTime(int)}, then converts the object * by a call to {@link Time#toLocalTime()}. * - * @todo Delegate to {@link ResultSet#getTime(int, Calendar)} instead. + * @todo java.sql.Time does not provide sub-second precision in its local time conversion. + * However, some databases support it. A better conversion function would be: + * {@code LocalTime.ofNanoOfDay(Math.multiplyExact(sqlTime.getTime(), 1_000_000L));} + * But it is less "standard". We should test across multiple database engines to verify the related impact. */ static final class AsLocalTime extends ValueGetter<LocalTime> { /** The unique instance of this accessor. */ @@ -371,7 +373,11 @@ public abstract class ValueGetter<T> { * This getter delegates to {@link ResultSet#getTime(int)}, converts the object by a call * to {@link Time#toLocalTime()} then apply the time zone offset. * - * @todo Delegate to {@link ResultSet#getTime(int, Calendar)} instead. + * @todo As for {@link AsLocalTime}, sub-second precision is lost here. + * + * Note: Timezone used on value insertion is lost here, we always return an UTC time. + * The main reason is that multiple database (Postgres, HSQLDB) engines convert times to UTC on save, + * so it is impossible to retrieve the information later. */ static final class AsOffsetTime extends ValueGetter<OffsetTime> { /** The unique instance of this accessor. */ @@ -380,10 +386,15 @@ public abstract class ValueGetter<T> { /** Fetches the value from the specified column in the given result set. */ @Override public OffsetTime getValue(InfoStatements stmts, ResultSet source, int columnIndex) throws SQLException { - final Time time = source.getTime(columnIndex); - if (time == null) return null; - final int offsetMinute = time.getTimezoneOffset(); - return time.toLocalTime().atOffset(ZoneOffset.ofHoursMinutes(offsetMinute / 60, offsetMinute % 60)); + final Object rawValue = source.getObject(columnIndex); + if (rawValue == null) return null; + // Workaround/shortcut for HSQLDB and H2 + else if (rawValue instanceof OffsetTime) return (OffsetTime) rawValue; + else { + final Time time = source.getTime(columnIndex); + final long timeNanoseconds = Math.multiplyExact(time.getTime(), 1_000_000L); + return LocalTime.ofNanoOfDay(timeNanoseconds).atOffset(ZoneOffset.UTC); + } } }
