Skip to content

Add a new JDBC driver parameter - timeZoneID#16756

Merged
tdcmeehan merged 1 commit intoprestodb:masterfrom
v-jizhang:timezone_parameter
Feb 24, 2022
Merged

Add a new JDBC driver parameter - timeZoneID#16756
tdcmeehan merged 1 commit intoprestodb:masterfrom
v-jizhang:timezone_parameter

Conversation

@v-jizhang
Copy link
Copy Markdown
Contributor

@v-jizhang v-jizhang commented Sep 17, 2021

Cherry-pick of trinodb/trino#7252

Co-authored-by: Shashikant Jaiswal shashi@okera.com

Test plan - Added test TestPrestoDriver$testTimeZoneIdParameter
Solves #16680

== RELEASE NOTES ==

General Changes
* Add a new JDBC driver parameter - timeZoneID
:doc:`/installation/jdbc`
:issue:`16680`

Copy link
Copy Markdown
Member

@imjalpreet imjalpreet left a comment

Choose a reason for hiding this comment

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

@v-jizhang Thanks for the contribution, changes look good. Some minor recommendations.

Also, please squash the commits.

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.

We can update the description to something like Timezone to be used for timestamp columns in query output

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.

Also changed timeZoneID to timeZoneId

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.

NIT: To follow camel case convention, I think we should name the class TimeZoneId

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.

Changed, thanks

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.

Same as above 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.

We can modify the return type of this method to String and return TimeZone.getDefault().getID() when the parameter is not set.

Comment on lines 160 to 166
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.

Suggestion: Since we are not using ZoneId, we can probably validate as follows: (NIT: also updated the parameter name timeZoneID to match with actual name in the exception message)

Suggested change
if (timezone.isPresent()) {
try {
// Validate timezone and return it.
ZoneId.of(timezone.get());
return timezone;
}
catch (DateTimeException e) {
throw new SQLException("Specified TimeZoneID is not supported: " + timezone.get(), e);
}
}
if (timezone.isPresent()) {
List<String> timeZoneIds = Arrays.asList(TimeZone.getAvailableIDs());
if (!timeZoneIds.contains(timezone.get())) {
throw new SQLException("Specified timeZoneID is not supported: " + timezone.get());
}
return timezone.get();
}

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.

This can be updated based on the changes to the method getTimeZoneID

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.

Done. Thanks.

@v-jizhang v-jizhang force-pushed the timezone_parameter branch 3 times, most recently from 09f5730 to 8e2256a Compare October 1, 2021 20:38
@rohanpednekar
Copy link
Copy Markdown
Contributor

@yingsu00 or @tdcmeehan Could you please give the final pass and approve this PR?

Cherry-pick of trinodb/trino#7252
Solves prestodb#16680

Co-authored-by: Shashikant Jaiswal <shashi@okera.com>
@rohanpednekar
Copy link
Copy Markdown
Contributor

@tdcmeehan could you please merge this PR, seems all checks have passed now.

@tdcmeehan tdcmeehan merged commit 79370de into prestodb:master Feb 24, 2022
@varungajjala varungajjala mentioned this pull request Mar 22, 2022
9 tasks
@asjadsyed asjadsyed mentioned this pull request Mar 23, 2022
9 tasks
@asjadsyed asjadsyed mentioned this pull request Apr 1, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants