Add Date, POSIXct and hms types by anderic1 · Pull Request #333 · r-dbi/RSQLite

anderic1 · 2020-12-16T15:07:43Z

Following #319 this PR adds support for Date, datetime and hms types. It depends on BH package version 1.75.0-0 which is not yet on CRAN.
The new option extended_types is set to TRUE by default in the dbConnect() method but could be set to FALSE to return to the old behaviour.

krlmlr · 2020-12-22T17:49:45Z

Thanks for looking into it. Is there a timeline for BH 1.75?

Can we keep extended_types = FALSE for now?

anderic1 · 2020-12-23T09:24:20Z

BH will be updated in the beginning of January. Some depending packages needed to be updated first. I hoped extended_types would be set to true, but I understand your concerns. You can go ahead and change it, I will not have access to my computer for the next few weeks.

…

Sent: Tuesday, December 22, 2020 at 6:49 PM From: "Kirill Müller" ***@***.***> To: "r-dbi/RSQLite" ***@***.***> Cc: "anderic1" ***@***.***>, "Author" ***@***.***> Subject: Re: [r-dbi/RSQLite] Add Date, POSIXct and hms types (#333) Thanks for looking into it. Is there a timeline for BH 1.75? Can we keep `extended_types = FALSE` for now? -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #333 (comment)

krlmlr

Thanks, I took a closer look.

Is the implementation in line with the discussion in #237?

krlmlr · 2020-12-27T09:21:55Z

    pkgconfig,
-    Rcpp (>= 0.12.7)
+    Rcpp (>= 0.12.7),
+    hms


I'm a bit hesitant to bring in hms as a dependency here. Can we move to Suggests and load only if a connection with extended types is requested?

krlmlr · 2020-12-27T09:24:06Z

+  if (with_alt_types) {
+      DATA_TYPE decl_dt = get_decl_data_type();
+      if (decl_dt == DT_DATE || decl_dt == DT_DATETIME || decl_dt == DT_TIME) {
+          return decl_dt;


Does this mean that a declared data type overrides what's stored in the columns?

krlmlr · 2020-12-27T09:24:36Z

+      }
+      return dateval;
+  } else {
+      return static_cast<double>(sqlite3_column_int(get_stmt(), get_j()));


What happens if the underlying data is an integer or a blob?

krlmlr · 2020-12-27T10:01:30Z

When done, we can enable many more DBItest tests that test for roundtrip for these data types.

anderic1 · 2020-12-27T15:48:33Z

Replies inline

Sent: Sunday, December 27, 2020 at 10:27 AM From: "Kirill Müller" ***@***.***> To: "r-dbi/RSQLite" ***@***.***> Cc: "anderic1" ***@***.***>, "Author" ***@***.***> Subject: Re: [r-dbi/RSQLite] Add Date, POSIXct and hms types (#333) @krlmlr commented on this pull request. Thanks, I took a closer look. Is the implementation in line with the discussion in #237?

I would say so, insofar as if the flag is not set everything will be like before. If (1) flag is set and (2) column is declared exactly modulo case, as 'date': a number will be interpreted as count of days since 1970-01-01, with class attribute 'Date' attached to it, ie an R date, and an iso string ('yyyy-mm-dd') will be converted to an R date. Everything else will return NA. Similarly for 'datetime'/'timestamp' -> POSIXct (UTC), and 'time' -> hms/difftime (unit=seconds).

> @@ -164,7 +165,20 @@ List SqliteResultImpl::get_column_info() { CharacterVector types(cache.ncols_); for (size_t i = 0; i < cache.ncols_; i++) { - types[i] = Rf_type2char(DbColumnStorage::sexptype_from_datatype(types_[i])); + switch(types_[i]) { + case DT_DATE: + types[i] = "Date"; + break; + case DT_DATETIME: + types[i] = "POSIXct"; + break; + case DT_TIME: + types[i] = "hms"; This needs to be the equivalent of `c("hms", "difftime")` .

Yes I was unsure about this one. POSIXct should similarly be c(POSIXct, POSIXt). Then the column should be a list column(?), but this also makes it harder to use. That's why I opted for a single element.

> @@ -43,14 +43,15 @@ Imports: memoise, methods, pkgconfig, - Rcpp (>= 0.12.7) + Rcpp (>= 0.12.7), + hms I'm a bit hesitant to bring in hms as a dependency here. Can we move to Suggests and load only if a connection with extended types is requested?

I brought it in because as the existing code was written, at one moment the c++ code calls new_hms() from the hms package on the column. This could of course be changed, then hms can be moved to suggests.

> { } DATA_TYPE SqliteColumnDataSource::get_data_type() const { + + if (with_alt_types) { + DATA_TYPE decl_dt = get_decl_data_type(); + if (decl_dt == DT_DATE || decl_dt == DT_DATETIME || decl_dt == DT_TIME) { + return decl_dt; Does this mean that a declared data type overrides what's stored in the columns?

Yes, but again only for the aforementioned column types, and if the flag is set. If the value cannot be converted NA is returned. If the flag is not set everything works like before.

> + int dt = get_column_type(); + + if (dt == SQLITE_TEXT) { + const char* dtstr = reinterpret_cast<const char*>(sqlite3_column_text(get_stmt(), get_j())); + double dateval; + + try { + bg::date dt(bg::from_simple_string(dtstr)); + bg::date_duration delta = dt - bg::date(1970, 1, 1); + dateval = static_cast<double>(delta.days()); + } catch (...) { + dateval = NA_REAL; + } + return dateval; + } else { + return static_cast<double>(sqlite3_column_int(get_stmt(), get_j())); What happens if the underlying data is an integer or a blob?

Integer works as expected. Blob however i have not tested. This is an oversight, my guess is that it will return 0 (ie 1970-01-01). A better option imo would be to return NA in this case.

…

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #333 (review)

anderic1 · 2021-01-04T14:06:34Z

I addressed all but one of the issues you mentioned and added warnings when blobs are encountered. I did not yet address the columninfo for time columns. As I mentioned earlier, how should this be returned? Should the whole column be coerced into a list column? Or perhaps add a third column as you alluded to in the beginning? For simplicity I think keeping just "hms"for time columns and "POSIXct" for datetime columns is the easiest and conveys sufficient information, but you be the judge.

krlmlr · 2021-01-04T16:34:42Z

Thanks. Could you please rerun (e.g. merge with master) when BH 1.75 becomes available on CRAN?

The class needs to be c("hms", "difftime"), for the column info the first component should be sufficient -- don't need to convert to a list here.

anderic1 · 2021-01-04T20:22:38Z

One last thing, would you mind if I changed the default value of extended_types to something like extended_types = getOption("RSQLite_extended_types", FALSE)? That way it can easily be enabled globally if one so wishes.

anderic1 · 2021-01-12T16:15:38Z

BH has been updated on cran.
Some checks are still failing, the Windows and Mac images have not yet been updated it seems.
The extended_types setting is set to FALSE by default, but can be overridden by setting options(RSQLite_extended_types = TRUE). Some changes were also needed to fix checks on all platforms.

krlmlr

Thanks, looks good. Just a few minor tweaks necessary.

Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>

anderic1 · 2021-01-19T12:49:56Z

I think we are done here. I committed your suggestions and you are right, it is better to be too strict and then loosen up than the other way around.

krlmlr · 2021-01-19T13:41:34Z

Thanks!

anderic1 added 3 commits December 16, 2020 14:25

handle DT_DATE, DT_DATETIME and DT_TIME

ec7553d

add global extended_types flag

ffb94d5

add tests

fc7f0e2

krlmlr reviewed Dec 27, 2020

View reviewed changes

This was referenced Dec 27, 2020

Coerce POSIXct to julian date when writing #237

Closed

Improve error message when writing dataframe with unsupported data type #246

Closed

krlmlr mentioned this pull request Dec 27, 2020

Fix test failures on Windows and Mac r-dbi/RMariaDB#117

Closed

3 tasks

anderic1 and others added 5 commits January 4, 2021 14:50

move hms to suggests

b39016b

add warnings and treat blobs as NA

b7c8526

improve documentation and extended_types FALSE by default

4f22014

add tests

8cfa58f

Merge branch 'master' into alt_types

cf0fbae

anderic1 added 4 commits January 12, 2021 15:25

allow enabling globally

e29776f

explicit types instead of auto

2a6d520

use boost string compare

cf5c01f

add origin and regenerate documentation

4bb3e97

krlmlr reviewed Jan 18, 2021

View reviewed changes

Comment thread R/connect.R Outdated

Comment thread R/connect.R Outdated

Comment thread src/DbColumnStorage.cpp Outdated

anderic1 and others added 4 commits January 18, 2021 16:13

remove possibility to enable globally

c7ffc61

Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>

regenerate documentation

379bd2a

condition on hms being available

f09fb5d

Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>

revert not calling new_hms

6d29b55

krlmlr merged commit 6bfe467 into r-dbi:master Jan 19, 2021

github-actions Bot locked as resolved and limited conversation to collaborators Jan 20, 2022

Uh oh!

Conversation

anderic1 commented Dec 16, 2020

Uh oh!

krlmlr commented Dec 22, 2020

Uh oh!

anderic1 commented Dec 23, 2020 via email

Uh oh!

krlmlr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

krlmlr Dec 27, 2020

Choose a reason for hiding this comment

Uh oh!

krlmlr Dec 27, 2020

Choose a reason for hiding this comment

Uh oh!

krlmlr Dec 27, 2020

Choose a reason for hiding this comment

Uh oh!

krlmlr commented Dec 27, 2020

Uh oh!

anderic1 commented Dec 27, 2020 via email

Uh oh!

anderic1 commented Jan 4, 2021

Uh oh!

krlmlr commented Jan 4, 2021

Uh oh!

anderic1 commented Jan 4, 2021

Uh oh!

anderic1 commented Jan 12, 2021

Uh oh!

krlmlr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anderic1 commented Jan 19, 2021

Uh oh!

krlmlr commented Jan 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants