You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2023/01/19 20:02:58 UTC
[GitHub] [calcite-avatica] wnob opened a new pull request, #205: [CALCITE-5487] Proper semantics for TIMESTAMP WITH LOCAL TIME ZONE
wnob opened a new pull request, #205:
URL: https://github.com/apache/calcite-avatica/pull/205
This is a work-in-progress as there are still some tests failing, but I believe the proper semantics are now codified in `AvaticaResultSetConversionsTest`. Was hoping to get some early feedback to see if I was barking up the wrong tree with these changes.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] wnob commented on a diff in pull request #205: [CALCITE-5487] Proper semantics for TIMESTAMP WITH LOCAL TIME ZONE
Posted by GitBox <gi...@apache.org>.
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 switch 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] julianhyde commented on a diff in pull request #205: [CALCITE-5487] Proper semantics for TIMESTAMP WITH LOCAL TIME ZONE
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] julianhyde commented on a diff in pull request #205: [CALCITE-5487] Proper semantics for TIMESTAMP WITH LOCAL TIME ZONE
Posted by GitBox <gi...@apache.org>.
julianhyde commented on code in PR #205:
URL: https://github.com/apache/calcite-avatica/pull/205#discussion_r1081983370
##########
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:
> 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
I don't know of any such bugs. But your changes do seem to make things worse - by ensuring that the nested `AvaticaResultSet` set does not get the right `timeZone`.
If there's a bug, log it.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] julianhyde commented on a diff in pull request #205: [CALCITE-5487] Proper semantics for TIMESTAMP WITH LOCAL TIME ZONE
Posted by GitBox <gi...@apache.org>.
julianhyde commented on code in PR #205:
URL: https://github.com/apache/calcite-avatica/pull/205#discussion_r1081816343
##########
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 think you just tore down a Chesterton's fence.
##########
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:
after this change the method name is no longer appropriate.
the purpose of this method is to access a SQL `TIMESTAMP` value as a string
you have added logic to access a SQL `TIMESTAMP WITH LOCAL TIME ZONE` value as a string. and paved over the old functionality, which we still need.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] wnob commented on a diff in pull request #205: [CALCITE-5487] Proper semantics for TIMESTAMP WITH LOCAL TIME ZONE
Posted by GitBox <gi...@apache.org>.
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.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] wnob commented on a diff in pull request #205: [CALCITE-5487] Proper semantics for TIMESTAMP WITH LOCAL TIME ZONE
Posted by GitBox <gi...@apache.org>.
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 `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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] wnob commented on a diff in pull request #205: [CALCITE-5487] Proper semantics for TIMESTAMP WITH LOCAL TIME ZONE
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] wnob commented on a diff in pull request #205: [CALCITE-5487] Proper semantics for TIMESTAMP WITH LOCAL TIME ZONE
Posted by GitBox <gi...@apache.org>.
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.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] wnob commented on a diff in pull request #205: [CALCITE-5487] Proper semantics for TIMESTAMP WITH LOCAL TIME ZONE
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] wnob commented on a diff in pull request #205: [CALCITE-5487] Proper semantics for TIMESTAMP WITH LOCAL TIME ZONE
Posted by GitBox <gi...@apache.org>.
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_ZONE` 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] julianhyde commented on pull request #205: [CALCITE-5487] Proper semantics for TIMESTAMP WITH LOCAL TIME ZONE
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on PR #205:
URL: https://github.com/apache/calcite-avatica/pull/205#issuecomment-1398777208
If it's not too much trouble, could you refactor the tests as a commit *before* the fix? It's disconcerting to see lots of test changes in a bug fix; I'd prefer to just see additions. The commit message cold be 'Refactor XxxTest'
I think that splitting accessor classes will help you achieve the 'hygiene' I referred to in https://issues.apache.org/jira/browse/CALCITE-5488.
The commit message should be the bug summary, i.e. '[CALCITE-5487] Support TIMESTAMP WITH LOCAL TIME ZONE in ResultSet'. Leaving the jira case number off makes it more difficult for the reviewer.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org