julianhyde commented on code in PR #205:
URL: https://github.com/apache/calcite-avatica/pull/205#discussion_r1081992148
##########
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.
Ah, I missed that. I guess the calendar parameter had some use in the
distant past.
Still, don't overuse the 'no tests broke' or 'intellij suggested a
refactoring' argument. By that argument you can justify renaming a variable
called 'black' to 'white' - intellij and tests don't care about class and
variable names - but it doesn't account for the confusion you create when you
don't make the code self-explanatory.
> Note that both a regular TIMESTAMP and a TIMESTAMP WITH LOCAL TIME ZONE
are represented in JDBC as TIMESTAMP
There are two separate decisions to make: how to represent the type, and how
to represent the value.
It seems that there is a way to represent the type: type = TIMESTAMP and
typeName = "TIMESTAMP_WITH_LOCAL_TIME_ZONE" or something like that.
How have you decided to represent the value? It's worth calling out
explicitly.
> I chose to re-use the existing timestamp getter with the addition of this
new fixedInstant parameter since it seemed like a straightforward addition.
It's not totally straightforward. Difference in interpretation is subtle.
And adding a boolean field and an extra couple of `if`s to some very simple
classes does make them significantly more complex.
So I would actually create a `class TimestampLtzAccessor` (maybe it would
extend `TimestampAccessor`, maybe not) to make everything explicit.
--
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
