wnob commented on code in PR #205: URL: https://github.com/apache/calcite-avatica/pull/205#discussion_r1081852921
########## core/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java: ########## @@ -238,12 +244,18 @@ protected Accessor createAccessor(ColumnMetaData columnMetaData, public abstract boolean next(); - /** Accesses a timestamp value as a string. + /** + * Accesses a timestamp value as a string. * The timestamp is in SQL format (e.g. "2013-09-22 22:30:32"), - * not Java format ("2013-09-22 22:30:32.123"). */ + * not Java format ("2013-09-22 22:30:32.123"). + * + * <p>Note that, when a TIMESTAMP is adjusted to a calendar, the offset is subtracted. + * Here, on the other hand, to adjust the string to the calendar (which only happens for type + * TIMESTAMP WITH LOCAL TIME ZONE), the offset is added. These are meant to be inverse operations. Review Comment: This is a private method, and all existing invocations of it pass it a null `Calendar`. If you open Avatica at head in IntelliJ, it suggests inlining the constant parameter. With my change, a `TIMESTAMP WITH LOCAL TIME ZONE` getter might pass it a non-null `Calendar`, and that's the only time it could be non-null. So, existing behavior cannot possibly be impacted by switching that minus to a plus. Note that both a regular `TIMESTAMP` and a `TIMESTAMP WITH LOCAL TIME ZONE` are represented in JDBC as `TIMESTAMP` (since there is no JDBC type for the latter). I chose to re-use the existing timestamp getter with the addition of this new `fixedInstant` parameter since it seemed like a straightforward addition. -- 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