FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-839: Fix support for ResultSet.getObject for TIMESTAMP_WITH_TIMEZONE by aiguofer · Pull Request #840 · apache/arrow-java · GitHub

/ arrow-java Public

GH-839: Fix support for ResultSet.getObject for TIMESTAMP_WITH_TIMEZONE - #840

Merged
lidavidm merged 7 commits into
apache:mainfrom
aiguofer:fix_support_for_timestamp_with_timezone
Oct 7, 2025
ConversationCommits (7)ChecksFiles changed
Merged

GH-839: Fix support for ResultSet.getObject for TIMESTAMP_WITH_TIMEZONE#840
lidavidm merged 7 commits into
apache:mainfrom
aiguofer:fix_support_for_timestamp_with_timezone

Conversation

aiguofer commented Sep 7, 2025 •
edited by lidavidm
Loading

Copy link
Copy Markdown
Contributor

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.

aiguofer commented Sep 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 timezone property on the driver (this is handled by Avatica) as well as changing the default JVM timezone.

lidavidm reviewed Sep 10, 2025
View reviewed changes
Comment thread .../flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/TimestampResultSetTest.java Outdated
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")),

lidavidm Sep 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Shouldn't the zone just be "+01:00"?

aiguofer Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

lidavidm Sep 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

aiguofer Sep 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

aiguofer Sep 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

lidavidm reviewed Sep 27, 2025
View reviewed changes

lidavidm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Just one last question, thanks

Comment thread ...ore/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightJdbcVectorSchemaRootResultSet.java Outdated
Cursor.Accessor accessor;
try {
accessor = accessorList.get(columnIndex - 1);
} catch (IndexOutOfBoundsException var3) {

lidavidm Sep 27, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit but why var3 and not e, exc, etc.? (This wasn't copied from a decompiler was it?)

aiguofer Sep 27, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ohhh I copy pasted this from navigating to the decompiled class in AvaticaResultSet... I'll change it :)

aiguofer Oct 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@lidavidm btw, this has been fixed already

lidavidm Oct 6, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the ping, sorry, I was out at back to back conferences and I'm still getting back into things 😅

lidavidm approved these changes Oct 6, 2025
View reviewed changes

lidavidm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Let's take this as a first step towards a more thoroughly tested/implemented driver. Thanks!

lidavidm commented Oct 6, 2025

Copy link
Copy Markdown
Member

It seems checkstyle is unhappy - perhaps compile locally? (The parallelism in CI makes the log really annoying to read...)

aiguofer force-pushed the fix_support_for_timestamp_with_timezone branch from d34df0f to 0702685 Compare October 6, 2025 17:37

aiguofer commented Oct 6, 2025

Copy link
Copy Markdown
Contributor Author

@lidavidm found the issues, just small docstring problems that have now been fixed.

lidavidm mentioned this pull request Oct 7, 2025
Open
lidavidm merged commit d31d9b0 into apache:main Oct 7, 2025
19 of 24 checks passed

thomas-chauvet commented Jan 5, 2026

Copy link
Copy Markdown

Thanks a lot for the fix !
Is there any timeline on when the fix will be released ?

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lidavidm lidavidm approved these changes
laurentgo Awaiting requested review from laurentgo laurentgo is a code owner
wgtmac Awaiting requested review from wgtmac wgtmac is a code owner
jbonofre Awaiting requested review from jbonofre jbonofre is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

3 participants

Back | FazBrowse Home | New Git URL