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.

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

ARROW-15795: [Java] Add a getter for the timeZone in timestamp with timezone vectors #12521

Closed
wants to merge 2 commits into from

Conversation

fabiencelier
Copy link
Contributor

https://issues.apache.org/jira/browse/ARROW-15795

I have added an abstract class for all the timestamp vectors with timezone

@github-actions
Copy link

@github-actions
Copy link

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@fabiencelier fabiencelier changed the title ARROW-15795: [Java] Add a getter for the timeZone in timestamp with timezone types ARROW-15795: [Java] Add a getter for the timeZone in timestamp with timezone vectors Feb 28, 2022
@fabiencelier
Copy link
Contributor Author

Let me know if there is anything else I can do for this PR

@BryanCutler
Copy link
Member

@fabiencelier we have tended to have a some code duplication to reduce class bloat for vectors, so I would prefer not to add an abstract class for this. You could get the timezone from a vector with

String tz = ((ArrowType.Timestamp) vector.getField().getFieldType().getType()).getTimezone();

which isn't pretty, so if you'd like to add a convenience method that does this in each of the timestamp with timezone vector classes, I think that would be ok.

@fabiencelier
Copy link
Contributor Author

Hello @BryanCutler

Thanks for your feedback on this ! I have reverted the change introducing an abstract class and simply added a getter for the time zone in the 4 vectors with timezone.

Copy link
Member

@BryanCutler BryanCutler left a comment

Choose a reason for hiding this comment

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

LGTM

@BryanCutler
Copy link
Member

Test failures look unrelated, merging to master. Thanks @fabiencelier !

@ursabot
Copy link

ursabot commented Mar 4, 2022

Benchmark runs are scheduled for baseline = 348057a and contender = 6734d0f. 6734d0f is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.42% ⬆️0.04%] test-mac-arm
[Finished ⬇️1.07% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.26% ⬆️0.09%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants