GH-839: Fix support for ResultSet.getObject for TIMESTAMP_WITH_TIMEZONE #840
Conversation
aiguofer
requested review from
jbonofre,
laurentgo,
lidavidm and
wgtmac
as code owners
|
I added an initial basic test with no assertions. This test ensures all methods succeed and prints out the results of each. This is a good starting point to add more tests and ensure we get the right values under all circumstances, including changing the |
| ImmutableList.of( | ||
| Field.nullable("no_tz", new ArrowType.Timestamp(TimeUnit.MILLISECOND, null)), | ||
| Field.nullable("utc", new ArrowType.Timestamp(TimeUnit.MILLISECOND, "UTC")), | ||
| Field.nullable("utc+1", new ArrowType.Timestamp(TimeUnit.MILLISECOND, "GMT+1:00")), |
There was a problem hiding this comment.
Shouldn't the zone just be "+01:00"?
There was a problem hiding this comment.
good question!! I couldn't really find a standard set of values. It seems Arrow doesn't really care at all and just keeps it as a string w/o any checks and it's up to the consumer to do something with it. I just used a value that worked with Java.
There was a problem hiding this comment.
I suppose the library does not validate it, but it is supposed to be either a timezone name or just an offset: https://github.com/apache/arrow/blob/37faf3e79512c2c036b004d80770bb522af035f1/format/Schema.fbs#L385-L395
There is Etc/GMT-1
There was a problem hiding this comment.
Oof... that complicates things even more since Java doesn't recognize just an offset, it wants it preceded by GMT.
Seems like we also need:
- logic to validate the offset/timezone values provided when creating an Arrow type
- logic to convert the string value of the arrow timezone to a Java Timezone or Offset object
There was a problem hiding this comment.
@lidavidm I updated the offsets to GMT+1 and GMT-1 for now, which seems to work correctly. I will create a separate issue for validation.
| Cursor.Accessor accessor; | ||
| try { | ||
| accessor = accessorList.get(columnIndex - 1); | ||
| } catch (IndexOutOfBoundsException var3) { |
There was a problem hiding this comment.
Nit but why var3 and not e, exc, etc.? (This wasn't copied from a decompiler was it?)
There was a problem hiding this comment.
ohhh I copy pasted this from navigating to the decompiled class in AvaticaResultSet... I'll change it :)
There was a problem hiding this comment.
@lidavidm btw, this has been fixed already
There was a problem hiding this comment.
Thanks for the ping, sorry, I was out at back to back conferences and I'm still getting back into things 😅
|
It seems checkstyle is unhappy - perhaps compile locally? (The parallelism in CI makes the log really annoying to read...) |
Turns out AvaticaSite.get does not account for TIMESTAMP_WITH_TIMEZONE types so we add support on an override function.
aiguofer
force-pushed
the
fix_support_for_timestamp_with_timezone
branch
from
d34df0f to
0702685
Compare
|
@lidavidm found the issues, just small docstring problems that have now been fixed. |
2 participants
What's Changed
Turns out AvaticaSite.get does not account for TIMESTAMP_WITH_TIMEZONE types so we add support on an override function.
Closes #818.
Closes #839.