wnob commented on code in PR #205:
URL: https://github.com/apache/calcite-avatica/pull/205#discussion_r1081880447
##########
core/src/main/java/org/apache/calcite/avatica/AvaticaResultSet.java:
##########
@@ -77,7 +78,9 @@ public AvaticaResultSet(AvaticaStatement statement,
ResultSetMetaData resultSetMetaData,
TimeZone timeZone,
Meta.Frame firstFrame) throws SQLException {
- super(timeZone);
+ // Initialize the parent ArrayFactory with the UTC time zone because
otherwise an array of
Review Comment:
I can't refute that with 100% certainty, but I will point out:
- Currently, at head, if you just make this 1 line change to pass
`UTC_CALENDAR` to the super constructor (here, the superclass being
`ArrayFactoryImpl`) and run all the tests, none of them fail.
- Alternatively, if you keep this 1 line as is, but go into
`AvaticaResultSetConversionTest` and delete the [`timeZone` property for the
connection][1], which is currently set to `"GMT"`, several `getString()` and
`getArray()` tests start to fail. The `getString()` failures are
self-explanatory, but the `getArray()` failures are tricky. They're
double-adjusting the timestamps:
- Once when they call [`getObject()`][2] (which invokes `getTimestamp()`
with the connection's default calendar, which was previously GMT but is now the
system default).
- Then, in [`ArrayImpl.equalContents()`][3] the arrays are converted to
result sets for traversal via [`Array.getResultSet()` ][4], which uses the same
time zone as the "factory" result set and eventually invokes `getObject()`
again, thus applying the time zone offset twice.
So, there's certainly an existing bug in the implementation of
`Array.getResultSet()` that only manifests when the array contains timestamps
and when the connection default time zone is not GMT, and this was not covered
by tests. By removing the explicit GMT `timeZone` for these tests, I'm covering
this case, and this seemed like the best solution. It's just hardcoding a UTC
time zone for the `ArrayFactoryImpl` parent of a `ResultSet`.
[1]:
https://github.com/apache/calcite-avatica/blob/dbe9b1d8c2e53474eb40cfaf5721aceca3bdb57f/core/src/test/java/org/apache/calcite/avatica/AvaticaResultSetConversionsTest.java#L1121
[2]:
https://github.com/apache/calcite-avatica/blob/810acf80771310431d7ef576f3404299ebb8eaf2/core/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java#L1432
[3]:
https://github.com/apache/calcite-avatica/blob/820edf6f653607afb5a2a280a32f315aff1f64cb/core/src/main/java/org/apache/calcite/avatica/util/ArrayImpl.java#L233
[4]:
https://docs.oracle.com/javase/8/docs/api/java/sql/Array.html#getResultSet--
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
