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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org