You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "mihaibudiu (via GitHub)" <gi...@apache.org> on 2024/02/27 01:31:52 UTC
[PR] [CALCITE-6282] Avatica ignores time precision when returning TIME results [calcite-avatica]
mihaibudiu opened a new pull request, #241:
URL: https://github.com/apache/calcite-avatica/pull/241
…
--
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
Re: [PR] [CALCITE-6282] Avatica ignores time precision when returning TIME results [calcite-avatica]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #241:
URL: https://github.com/apache/calcite-avatica/pull/241#discussion_r1530777139
##########
core/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java:
##########
@@ -955,10 +955,12 @@ protected Number getNumber() throws SQLException {
*/
static class TimeFromNumberAccessor extends NumberAccessor {
private final Calendar localCalendar;
+ private final int precision;
- TimeFromNumberAccessor(Getter getter, Calendar localCalendar) {
+ TimeFromNumberAccessor(Getter getter, Calendar localCalendar, int precision) {
super(getter, 0);
Review Comment:
I will rename it to "scale"
--
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
Re: [PR] [CALCITE-6282] Avatica ignores time precision when returning TIME results [calcite-avatica]
Posted by "F21 (via GitHub)" <gi...@apache.org>.
F21 commented on PR #241:
URL: https://github.com/apache/calcite-avatica/pull/241#issuecomment-2010653717
I would suggest we temporary comment out the tests in Calcite and renable them when it's upgrade to Avatica 1.25.0. See Julian's suggestion: https://lists.apache.org/thread/b7kb393yvj10rbmsxv4bh076bk2ncx58
--
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
Re: [PR] [CALCITE-6282] Avatica ignores time precision when returning TIME results [calcite-avatica]
Posted by "YiwenWu (via GitHub)" <gi...@apache.org>.
YiwenWu commented on code in PR #241:
URL: https://github.com/apache/calcite-avatica/pull/241#discussion_r1529568026
##########
core/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java:
##########
@@ -1220,7 +1227,7 @@ static class TimestampAccessor extends ObjectAccessor {
}
final long unix =
DateTimeUtils.sqlTimestampToUnixTimestamp(timestamp, localCalendar);
- return timestampAsString(unix, null);
+ return timestampAsString(unix, null, this.precision);
Review Comment:
For `cast(TIME '12:42:25.34' as TIME(2))` case, whether to consider
`DateTimeUtils.unixTimeToSqlTime` corresponding changes
##########
core/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java:
##########
@@ -955,10 +955,12 @@ protected Number getNumber() throws SQLException {
*/
static class TimeFromNumberAccessor extends NumberAccessor {
private final Calendar localCalendar;
+ private final int precision;
- TimeFromNumberAccessor(Getter getter, Calendar localCalendar) {
+ TimeFromNumberAccessor(Getter getter, Calendar localCalendar, int precision) {
super(getter, 0);
Review Comment:
I have a question, should the 2 in `Time(2)` be `scale` or `precision`?
--
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
Re: [PR] [CALCITE-6282] Avatica ignores time precision when returning TIME results [calcite-avatica]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #241:
URL: https://github.com/apache/calcite-avatica/pull/241
--
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
Re: [PR] [CALCITE-6282] Avatica ignores time precision when returning TIME results [calcite-avatica]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #241:
URL: https://github.com/apache/calcite-avatica/pull/241#discussion_r1531463054
##########
core/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java:
##########
@@ -955,10 +955,12 @@ protected Number getNumber() throws SQLException {
*/
static class TimeFromNumberAccessor extends NumberAccessor {
private final Calendar localCalendar;
+ private final int precision;
- TimeFromNumberAccessor(Getter getter, Calendar localCalendar) {
+ TimeFromNumberAccessor(Getter getter, Calendar localCalendar, int precision) {
super(getter, 0);
Review Comment:
I guess I have to leave it as `precision`, since both the columnMetadataFields which originate this value call it `precision`, and the helper functions that manipulate this field also call it `precision` (as you point out in your other comment). It would be too confusing to use different names for this value in different parts of the codebase.
I propose we file a new issue if we want to modify the field name to `scale`, to make sure that we cover all the software layers involved. In the meantime it would be great to merge this PR because it fixes a genuine bug.
--
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
Re: [PR] [CALCITE-6282] Avatica ignores time precision when returning TIME results [calcite-avatica]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #241:
URL: https://github.com/apache/calcite-avatica/pull/241#issuecomment-2009956804
So what's the plan about merging this PR? Do I have to make somehow the CI work?
--
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