Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CALCITE-6282] Avatica ignores time precision when returning TIME results #241

Merged
merged 1 commit into from
Mar 29, 2024
Merged

[CALCITE-6282] Avatica ignores time precision when returning TIME results #241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 26 additions & 17 deletions core/src/main/java/org/apache/calcite/avatica/util/AbstractCursor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ protected Accessor createAccessor(ColumnMetaData columnMetaData,
case PRIMITIVE_INT:
case INTEGER:
case NUMBER:
return new TimeFromNumberAccessor(getter, localCalendar);
return new TimeFromNumberAccessor(getter, localCalendar, columnMetaData.precision);
case JAVA_SQL_TIME:
return new TimeAccessor(getter, localCalendar);
default:
Expand All @@ -165,11 +165,11 @@ protected Accessor createAccessor(ColumnMetaData columnMetaData,
case PRIMITIVE_LONG:
case LONG:
case NUMBER:
return new TimestampFromNumberAccessor(getter, localCalendar);
return new TimestampFromNumberAccessor(getter, localCalendar, columnMetaData.precision);
case JAVA_SQL_TIMESTAMP:
return new TimestampAccessor(getter, localCalendar);
return new TimestampAccessor(getter, localCalendar, columnMetaData.precision);
case JAVA_UTIL_DATE:
return new TimestampFromUtilDateAccessor(getter, localCalendar);
return new TimestampFromUtilDateAccessor(getter, localCalendar, columnMetaData.precision);
default:
throw new AssertionError("bad " + columnMetaData.type.rep);
}
Expand Down Expand Up @@ -241,11 +241,11 @@ protected Accessor createAccessor(ColumnMetaData columnMetaData,
/** 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"). */
private static String timestampAsString(long v, Calendar calendar) {
private static String timestampAsString(long v, Calendar calendar, int precision) {
if (calendar != null) {
v -= calendar.getTimeZone().getOffset(v);
}
return DateTimeUtils.unixTimestampToString(v);
return DateTimeUtils.unixTimestampToString(v, precision);
}

/** Accesses a date value as a string, e.g. "2013-09-22". */
Expand All @@ -255,11 +255,11 @@ private static String dateAsString(int v, Calendar calendar) {
}

/** Accesses a time value as a string, e.g. "22:30:32". */
private static String timeAsString(int v, Calendar calendar) {
private static String timeAsString(int v, Calendar calendar, int precision) {
if (calendar != null) {
v -= calendar.getTimeZone().getOffset(v);
}
return DateTimeUtils.unixTimeToString(v);
return DateTimeUtils.unixTimeToString(v, precision);
}

/** Implementation of {@link Cursor.Accessor}. */
Expand Down Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a question, should the 2 in Time(2) be scale or precision?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will rename it to "scale"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.localCalendar = localCalendar;
this.precision = precision;
}

@Override public Object getObject() throws SQLException {
Expand Down Expand Up @@ -986,7 +988,7 @@ static class TimeFromNumberAccessor extends NumberAccessor {
if (v == null) {
return null;
}
return timeAsString(v.intValue(), null);
return timeAsString(v.intValue(), null, this.precision);
}

protected Number getNumber() throws SQLException {
Expand All @@ -1013,10 +1015,12 @@ protected Number getNumber() throws SQLException {
*/
static class TimestampFromNumberAccessor extends NumberAccessor {
private final Calendar localCalendar;
private final int precision;

TimestampFromNumberAccessor(Getter getter, Calendar localCalendar) {
TimestampFromNumberAccessor(Getter getter, Calendar localCalendar, int precision) {
super(getter, 0);
this.localCalendar = localCalendar;
this.precision = precision;
}

@Override public Object getObject() throws SQLException {
Expand Down Expand Up @@ -1052,7 +1056,7 @@ static class TimestampFromNumberAccessor extends NumberAccessor {
if (v == null) {
return null;
}
return timestampAsString(v.longValue(), null);
return timestampAsString(v.longValue(), null, this.precision);
}

protected Number getNumber() throws SQLException {
Expand Down Expand Up @@ -1155,7 +1159,8 @@ static class TimeAccessor extends ObjectAccessor {
return null;
}
final int unix = DateTimeUtils.sqlTimeToUnixTime(time, localCalendar);
return timeAsString(unix, null);
// java.sql.Time only supports a precision of 0
return timeAsString(unix, null, 0);
}

@Override public long getLong() throws SQLException {
Expand All @@ -1178,10 +1183,12 @@ static class TimeAccessor extends ObjectAccessor {
*/
static class TimestampAccessor extends ObjectAccessor {
private final Calendar localCalendar;
private final int precision;

TimestampAccessor(Getter getter, Calendar localCalendar) {
TimestampAccessor(Getter getter, Calendar localCalendar, int precision) {
super(getter);
this.localCalendar = localCalendar;
this.precision = precision;
}

@Override public Timestamp getTimestamp(Calendar calendar) throws SQLException {
Expand Down Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For cast(TIME '12:42:25.34' as TIME(2)) case, whether to consider
DateTimeUtils.unixTimeToSqlTime corresponding changes

}

@Override public long getLong() throws SQLException {
Expand All @@ -1241,11 +1248,13 @@ static class TimestampAccessor extends ObjectAccessor {
*/
static class TimestampFromUtilDateAccessor extends ObjectAccessor {
private final Calendar localCalendar;
private final int precision;

TimestampFromUtilDateAccessor(Getter getter,
Calendar localCalendar) {
Calendar localCalendar, int precision) {
super(getter);
this.localCalendar = localCalendar;
this.precision = precision;
}

@Override public Timestamp getTimestamp(Calendar calendar) throws SQLException {
Expand Down Expand Up @@ -1282,7 +1291,7 @@ static class TimestampFromUtilDateAccessor extends ObjectAccessor {
return null;
}
final long unix = DateTimeUtils.utilDateToUnixTimestamp(date, localCalendar);
return timestampAsString(unix, null);
return timestampAsString(unix, null, this.precision);
}

@Override public long getLong() throws SQLException {
Expand Down
24 changes: 23 additions & 1 deletion core/src/test/java/org/apache/calcite/avatica/util/TimeFromNumberAccessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public class TimeFromNumberAccessorTest {
final AbstractCursor.Getter getter = new LocalGetter();
localCalendar = Calendar.getInstance(TimeZone.getDefault(), Locale.ROOT);
instance = new AbstractCursor.TimeFromNumberAccessor(getter,
localCalendar);
localCalendar, 0);
}

/**
Expand Down Expand Up @@ -156,4 +156,26 @@ private class LocalGetter implements AbstractCursor.Getter {
return value == null;
}
}

/**
* Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6282">[CALCITE-6282]
* Avatica ignores time precision when returning TIME results</a>. */
@Test public void testPrecision() throws SQLException {
final AbstractCursor.Getter getter = new AbstractCursor.Getter() {
@Override
public Object getObject() throws SQLException {
// This is the representation of TIME '12:42:25.34'
return 45745340;
}

@Override
public boolean wasNull() throws SQLException {
return false;
}
};
AbstractCursor.TimeFromNumberAccessor accessor = new AbstractCursor.TimeFromNumberAccessor(
getter, null, 2);
String string = accessor.getString();
assertThat(string, is("12:42:25.34"));
}
}
2 changes: 1 addition & 1 deletion core/src/test/java/org/apache/calcite/avatica/util/TimestampAccessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public class TimestampAccessorTest {
@Before public void before() {
final AbstractCursor.Getter getter = new LocalGetter();
localCalendar = Calendar.getInstance(TimeZone.getDefault(), Locale.ROOT);
instance = new AbstractCursor.TimestampAccessor(getter, localCalendar);
instance = new AbstractCursor.TimestampAccessor(getter, localCalendar, 0);
}

/**
Expand Down
31 changes: 30 additions & 1 deletion core/src/test/java/org/apache/calcite/avatica/util/TimestampFromNumberAccessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ public class TimestampFromNumberAccessorTest {
// UTC: 2014-09-30 15:28:27.356
private static final long DST_INSTANT = 1412090907356L;
private static final String DST_STRING = "2014-09-30 15:28:27";
// With precision 2
private static final String DST_STRING_2 = "2014-09-30 15:28:27.35";
// With precision 3
private static final String DST_STRING_3 = "2014-09-30 15:28:27.356";

// UTC: 1500-04-30 12:00:00.123 (JULIAN CALENDAR)
private static final long PRE_GREG_INSTANT = -14821444799877L;
Expand All @@ -56,7 +60,7 @@ public class TimestampFromNumberAccessorTest {
@Before public void before() {
final AbstractCursor.Getter getter = new LocalGetter();
localCalendar = Calendar.getInstance(TimeZone.getDefault(), Locale.ROOT);
instance = new AbstractCursor.TimestampFromNumberAccessor(getter, localCalendar);
instance = new AbstractCursor.TimestampFromNumberAccessor(getter, localCalendar, 0);
}

/**
Expand All @@ -72,6 +76,31 @@ public class TimestampFromNumberAccessorTest {
is(Timestamp.valueOf("1500-04-30 12:00:00")));
}

/**
* Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-6282">
* [CALCITE-6282] Avatica ignores time precision when returning TIME results</a>. */
@Test public void testPrecision() throws SQLException {
final AbstractCursor.Getter getter = new AbstractCursor.Getter() {
@Override
public Object getObject() throws SQLException {
return DST_INSTANT;
}

@Override
public boolean wasNull() throws SQLException {
return false;
}
};
AbstractCursor.TimestampFromNumberAccessor accessor =
new AbstractCursor.TimestampFromNumberAccessor(getter, null, 2);
String string = accessor.getString();
assertThat(string, is(DST_STRING_2));
accessor = new AbstractCursor.TimestampFromNumberAccessor(
getter, null, 3);
string = accessor.getString();
assertThat(string, is(DST_STRING_3));
}

/**
* Test {@code getDate()} handles time zone conversions relative to the local calendar and not
* UTC.
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/org/apache/calcite/avatica/util/TimestampFromUtilDateAccessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public class TimestampFromUtilDateAccessorTest {
@Before public void before() {
final AbstractCursor.Getter getter = new LocalGetter();
localCalendar = Calendar.getInstance(TimeZone.getDefault(), Locale.ROOT);
instance = new AbstractCursor.TimestampFromUtilDateAccessor(getter, localCalendar);
instance = new AbstractCursor.TimestampFromUtilDateAccessor(getter, localCalendar, 0);
}

/**
Expand Down
Loading