[jira] [Comment Edited] (CALCITE-5488) Calcite double-adjusts timestamps when calling Array.getResultSet()

["Julian Hyde (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CALCITE-5488?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17679256#comment-17679256 ] 

Julian Hyde edited comment on CALCITE-5488 at 1/20/23 6:31 PM:
---------------------------------------------------------------

Thanks for logging this.

Is this bug in Avatica? If so, please set the component; and if necessary adjust the subject.

This kind of double-conversion could perhaps be avoided using hygiene:
 * Never add or subtract an offset to a {{java.sql.Timestamp}} to create a different {{java.sql.Timestamp}} with an offset (because a {{java.sql.Timestamp}} represents an instant it is impossible for it to be 'in the wrong time zone').
 * If a variable has type {{long}}, its name should make clear whether it is an instant or a local time, and if a local time, relative to which time zone (the time zone of the local JVM, of the server, etc.). 
 * When converting a {{long}} to a {{java.sql.Timestamp}} be explicit that you are using the right algorithm (add/subtract timezone offset, or copy the UTC-based instant value) based on the type of the {{long}}
 * Avoid using the term "timestamp"; say {{TIMESTAMP}} when you mean the SQL type, which contains a local datetime, and {{java.sql.Timestamp}} when you mean the Java type. JDBC types refer to the constants in {{java.sql.Types}} and align with the SQL type system (albeit with omissions such as {{TIMESTAMP WITH LOCAL TIME ZONE}}.

The fix for this bug should identify where those rules of hygiene were breached.


was (Author: julianhyde):
Thanks for logging this.

Is this bug in Avatica? If so, please set the component; and if necessary adjust the subject.

This kind of double-conversion could perhaps be avoided using hygiene:
 * Never add or subtract an offset to a {{java.sql.Timestamp}} to create a different {{java.sql.Timestamp}} with an offset (because a {{java.sql.Timestamp}} represents an instant it is impossible for it to be 'in the wrong time zone').
 * If a variable has type {{long}}, its name should make clear whether it is an instant or a local time, and if a local time, relative to which time zone (the time zone of the local JVM, of the server, etc.). 
 * When converting a {{long}} to a {{java.sql.Timestamp}} be explicit that you are using the right algorithm (add/subtract timezone offset, or copy the UTC-based instant value) based on the type of the {{long}

The fix for this bug should identify where those rules of hygiene were breached.

> Calcite double-adjusts timestamps when calling Array.getResultSet()
> -------------------------------------------------------------------
>
>                 Key: CALCITE-5488
>                 URL: https://issues.apache.org/jira/browse/CALCITE-5488
>             Project: Calcite
>          Issue Type: Bug
>            Reporter: Will Noble
>            Assignee: Will Noble
>            Priority: Minor
>
> If you go into {{AvaticaResultSetConversionTest}} and delete the [timeZone|https://github.com/apache/calcite-avatica/blob/dbe9b1d8c2e53474eb40cfaf5721aceca3bdb57f/core/src/test/java/org/apache/calcite/avatica/AvaticaResultSetConversionsTest.java#L1121] property for the connection, which is currently set to "GMT", several {{getString()}} and {{getArray()}} tests start to fail. The {{getString()}} failures are self-explanatory -- the expected value has become incorrect because of the time zone change -- but the {{getArray()}} failures are tricky. They're double-adjusting the timestamps:
> * Once when they call [getObject()|https://github.com/apache/calcite-avatica/blob/810acf80771310431d7ef576f3404299ebb8eaf2/core/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java#L1432] (which invokes {{getTimestamp()}} with the connection's default calendar, which was previously GMT but is now the system default).
> * Then, in [ArrayImpl.equalContents()|https://github.com/apache/calcite-avatica/blob/820edf6f653607afb5a2a280a32f315aff1f64cb/core/src/main/java/org/apache/calcite/avatica/util/ArrayImpl.java#L233] the arrays are converted to result sets for traversal via [Array.getResultSet()|https://docs.oracle.com/javase/8/docs/api/java/sql/Array.html#getResultSet--] , which uses the same time zone as the "factory" result set and eventually invokes {{getObject()}} again, thus applying the time zone offset twice.
> This is a bug in the implementation of {{Array.getResultSet()}} that only manifests when the array contains timestamps and when the connection default time zone is anything besides GMT, and this is not currently covered by tests. It's easy to cause the failure, however, by changing the connection default time zone for {{AvaticaResultSetConversionTest}} to anything besides GMT or UTC.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)