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

new param timezone_out to control the timezone returned to R #294

Merged
merged 11 commits into from Nov 8, 2019

Conversation

@shrektan
Copy link
Contributor

shrektan commented Aug 26, 2019

Closes #275

This PR allows user to define a new param timezone_out so that the timezone of the returned object can be local timezone or whatever other than UTC.

I believe it is very convinient for many users because usually the timezones on the local machine and the server are the same. By forcing the timezone to be UTC leads to sort-of confusing for users, especially those who are not familiar with timezone things.

An Example

Note this result now displays the same as what's in SQL. But without this functionality, the user will get "2018-12-31 20:00:00 UTC" and have to convert it to "Asia/Shanghai" later by themselves, which is verbose and inconvinient.

con <- DBI::dbConnect(
  odbc::odbc(),
  server = '#hide#', database = '#hide#', uid = '#hide#', pwd = '#hide#',
  encoding = 'GB2312', driver = 'SQL Server', port = #hide#,
  timezone = 'Asia/Shanghai', timezone_out = 'Asia/Shanghai'
)
out <- DBI::dbGetQuery(con, "select cast('2019-01-01 04:00:00' as datetime) as datetime")
str(out)
#> 'data.frame':    1 obs. of  1 variable:
#>  $ datetime: POSIXct, format: "2019-01-01 04:00:00"
attributes(out$datetime)  
#> $class
#> [1] "POSIXct" "POSIXt" 
#> 
#> $tzone
#> [1] "Asia/Shanghai"
DBI::dbDisconnect(con)

Created on 2019-08-26 by the reprex package (v0.2.1)

@krlmlr
krlmlr approved these changes Oct 8, 2019
Copy link
Member

krlmlr left a comment

Thanks, looks good! I believe this is the only safe way to implement this. Ideally we'd use timezone_out = timezone as default but this seems very risky for scripts that already set a timezone argument.

src/odbc_result.cpp Outdated Show resolved Hide resolved
@shrektan

This comment has been minimized.

Copy link
Contributor Author

shrektan commented Oct 9, 2019

@krlmlr It should be correct now!

db_tz <- 'Asia/Shanghai' 
user_tz <- 'America/Los_Angeles'
con <- DBI::dbConnect(
  odbc::odbc(),
  server = '127.0.0.1', database = 'tempdb', uid = 'sa', pwd = 'Abc_123_Bcd',
  encoding = 'GB2312', driver = 'ODBC Driver 13 for SQL Server', port = 1433,
  timezone = db_tz, timezone_out = user_tz
)
out <- DBI::dbGetQuery(con, "select cast('2019-01-01 04:00:00' as datetime) as datetime, 
                       cast('2019-01-01' as date) as date")
str(out)
#> 'data.frame':    1 obs. of  2 variables:
#>  $ datetime: POSIXct, format: "2018-12-31 12:00:00"
#>  $ date    : Date, format: "2019-01-01"
attributes(out$datetime)
#> $class
#> [1] "POSIXct" "POSIXt" 
#> 
#> $tzone
#> [1] "America/Los_Angeles"
lubridate::with_tz(out$datetime, db_tz)
#> [1] "2019-01-01 04:00:00 CST"
print(out$date)
#> [1] "2019-01-01"
DBI::dbDisconnect(con)

Created on 2019-10-10 by the reprex package (v0.3.0)

shrektan added 3 commits Oct 9, 2019
Merge branch 'master' of https://github.com/r-dbi/odbc into timezone_out

# Conflicts:
#	src/odbc_result.cpp
@shrektan

This comment has been minimized.

Copy link
Contributor Author

shrektan commented Oct 22, 2019

The failed check is only happened on R-devel and is caused by a warning "checking top-level files ... WARNING", which seems not related to this PR.

Moreover, just to note that now it's possible to have an MS SQL Server inside of a docker container, making the tests more easily. See quickstart-install-connect-docker for details. 😃

@jimhester

This comment has been minimized.

Copy link
Member

jimhester commented Oct 24, 2019

Nice, that is useful!

@jimhester jimhester merged commit fc734a9 into r-dbi:master Nov 8, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@jimhester

This comment has been minimized.

Copy link
Member

jimhester commented Nov 8, 2019

Awesome, great work!

Thanks a bunch!

@shrektan shrektan deleted the shrektan:timezone_out branch Nov 9, 2019
@wibeasley wibeasley mentioned this pull request Dec 9, 2019
0 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.