Skip to content

Added a new JDBC driver parameter - 'TimeZoneID'#7252

Closed
Shashi-Jaiswal wants to merge 1 commit intotrinodb:masterfrom
Shashi-Jaiswal:timezone-id-jdbc-parameter
Closed

Added a new JDBC driver parameter - 'TimeZoneID'#7252
Shashi-Jaiswal wants to merge 1 commit intotrinodb:masterfrom
Shashi-Jaiswal:timezone-id-jdbc-parameter

Conversation

@Shashi-Jaiswal
Copy link
Copy Markdown

@Shashi-Jaiswal Shashi-Jaiswal commented Mar 11, 2021

Hello Team,
I have added a new JDBC driver parameter: TimeZoneID. This parameter may be specified as part of URI
or as part of properties passed to DriverManager. Both of the following examples are equivalent:

// URL parameter
String url = "jdbc:trino://example.net:8080/hive/sales?TimeZoneID=US/Eastern";
Connection connection = DriverManager.getConnection(url);

// Properties
String url = "jdbc:trino://example.net:8080/hive/sales";
Properties properties = new Properties();
properties.setProperty("TimeZoneID", "US/Eastern");
Connection connection = DriverManager.getConnection(url, properties);

Could you please let me know where can I write tests for this change? And if there is anything else needed for this pull request to be merged?

Fixes #7158

@cla-bot
Copy link
Copy Markdown

cla-bot bot commented Mar 11, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

{
public TimeZoneID()
{
super("TimeZoneID", NOT_REQUIRED, ALLOWED, STRING_CONVERTER);
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.

"Timezone" to be consistent with CLI's --timezone

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.

Let's make it lowercase timezone to be consistent with other properties (all are lowercase except Kerberos and SSL)

public Optional<String> getTimeZoneID()
throws SQLException
{
Optional<String> tzId = TIMEZONE_ID.getValue(properties);
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.

avoid abbreviations, call it just timezone

Comment on lines +204 to +208
List<String> ids = Arrays.asList(java.util.TimeZone.getAvailableIDs());
if (ids.contains(tzId.get())) {
return tzId;
}
throw new SQLException("Specified TimeZoneID is not supported");
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.

Use ZoneId for validation and then just return it

if (timezone.isPresent()) {
   try {
     return ZoneId.of(timezone.get());
   }
   catch (DateTimeException | ZoneRulesException e) {
    throw new SQLException("Specified timezone is not supported: " + timezone.get(), e);
  }
}

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.

Also, we don't need to return Optional at all, since we can return ZoneId.systemDefault() here when parameter is not set.

@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 12, 2021

For the test, see io.trino.jdbc.TestTrinoDriver#testSetTimeZoneId.

the test would be similar, just the initialization would go into connection string rather than using connection.unwrap(TrinoConnection.class).setTimeZoneId("UTC");

v-jizhang added a commit to v-jizhang/presto that referenced this pull request Jan 29, 2022
Cherry-pick of trinodb/trino#7252
Solves prestodb#16680

Co-authored-by: Shashikant Jaiswal <shashi@okera.com>
tdcmeehan pushed a commit to prestodb/presto that referenced this pull request Feb 24, 2022
Cherry-pick of trinodb/trino#7252
Solves #16680

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

👋 @Shashi-Jaiswal - this PR is inactive and doesn't seem to be under development. If you'd like to continue work on this at any point in the future, feel free to re-open.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Add Timezone parameter in JDBC URI

6 participants