automatically convert posixlt to posixct in set/:=/<- (#6299)

* automatically convert posixlt to posixct in set * remove ws * update warning * add tests * add news * Update NEWS.md * update warning * add breaking change * move to dev NEWS --------- Co-authored-by: Michael Chirico <[email protected]> Co-authored-by: Michael Chirico <[email protected]>
Rdatatable · Aug 28, 2024 · be97437 · be97437
1 parent b9cab71
commit be97437
Show file tree

Hide file tree

Showing 6 changed files with 23 additions and 6 deletions.
diff --git a/NEWS.md b/NEWS.md
@@ -2,6 +2,10 @@
 
 # data.table [v1.16.99](https://github.com/Rdatatable/data.table/milestone/31)  (in development)
 
+## POTENTIALLY BREAKING CHANGES
+
+1. In `DT[, variable := value]`, when value is class `POSIXlt`, we automatically coerce it to class `POSIXct` instead, [#1724](https://github.com/Rdatatable/data.table/issues/1724). Thanks to @linzhp for the report, and Benjamin Schwendinger for the fix.
+
 ## NOTES
 
 1. Tests run again when some Suggests packages are missing, [#6411](https://github.com/Rdatatable/data.table/issues/6411). Thanks @aadler for the note and @MichaelChirico for the fix.

diff --git a/R/data.table.R b/R/data.table.R
@@ -1267,11 +1267,6 @@ replace_dot_alias = function(e) {
   }  # end of  if !missing(j)
 
   SDenv = new.env(parent=parent.frame())
-  # taking care of warnings for posixlt type, #646
-  SDenv$strptime = function(x, ...) {
-    warningf("strptime() usage detected and wrapped with as.POSIXct(). This is to minimize the chance of assigning POSIXlt columns, which use 40+ bytes to store one date (versus 8 for POSIXct). Use as.POSIXct() (which will call strptime() as needed internally) to avoid this warning.")
-    as.POSIXct(base::strptime(x, ...))
-  }
 
   syms = all.vars(jsub)
   syms = syms[ startsWith(syms, "..") ]

diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw
@@ -8619,7 +8619,7 @@ ll = list(a=as.POSIXlt("2015-01-01", tz='UTC'), b=1:5)
 test(1612.1, as.data.table(ll), data.table(a=as.POSIXct("2015-01-01", tz='UTC'), b=1:5), warning="POSIXlt column type detected")
 dt = data.table(d1="1984-03-17")
 ans = data.table(d1="1984-03-17", d2=as.POSIXct("1984-03-17", tz='UTC'))
-test(1612.2, dt[, d2 := strptime(d1, "%Y-%m-%d", tz='UTC')], ans, warning="strptime() usage detected and wrapped with as.POSIXct()")
+test(1612.2, dt[, d2 := strptime(d1, "%Y-%m-%d", tz='UTC')], ans, warning="POSIXlt detected and converted to POSIXct")
 ll = list(a=as.POSIXlt("2015-01-01"), b=2L)
 test(1612.3, setDT(ll), error="Column 1 has class 'POSIXlt'")
 
@@ -19040,3 +19040,14 @@ test(2278.3, set(copy(dt), 0L,          "b", NA),         copy(dt)[0L, b := NA])
 test(2278.4, set(copy(dt), NA_integer_, "b", logical(0)), copy(dt)[NA_integer_, b := logical(0)])
 test(2278.5, set(copy(dt), integer(0),  "b", numeric(0)), copy(dt)[integer(0),  b := numeric(0)])
 test(2278.6, { set(dt, 0L, "b", logical(0)); set(dt, 1L, "a", 2L); dt }, data.table(a=2L, b=NA))
+
+# convert POSIXlt to POSIXct in set and := #1724
+dt = data.table(a=1:2)
+dates_char = c("2024-01-01", "2024-01-02")
+dates = as.POSIXlt(dates_char, tz="UTC")
+ans = data.table(a=1:2, dates=as.POSIXct(dates))
+test(2279.1, copy(dt)[, dates := dates], ans, warning="POSIXlt.*converted to POSIXct")
+test(2279.2, set(copy(dt), j="dates", value=dates), ans, warning="POSIXlt.*converted to POSIXct")
+test(2279.3, {dt1 = copy(dt); dt1$dates=dates; dt1}, ans, warning="POSIXlt.*converted to POSIXct")
+test(2279.4, copy(dt)[, dates := strptime(dates_char, "%Y-%m-%d", tz="UTC")], ans, warning="POSIXlt.*converted to POSIXct")
+
diff --git a/src/assign.c b/src/assign.c
@@ -429,6 +429,10 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values)
   }
   if (any_duplicated(cols,FALSE)) error(_("Can't assign to the same column twice in the same query (duplicates detected)."));
   if (!isNull(newcolnames) && !isString(newcolnames)) error(_("newcolnames is supplied but isn't a character vector"));
+  if (Rf_inherits(values, "POSIXlt")) {
+    warning(_("Values of type POSIXlt detected and converted to POSIXct. We do not recommend the use of POSIXlt at all because it typically takes more than 6 times the storage as an equivalent POSIXct column. Use as.POSIXct() to avoid this warning."));
+    values = PROTECT(eval(PROTECT(lang2(sym_as_posixct, values)), R_GlobalEnv)); protecti+=2;
+  }
   bool RHS_list_of_columns = TYPEOF(values)==VECSXP && length(cols)>1;  // initial value; may be revised below
   if (verbose) Rprintf(_("RHS_list_of_columns == %s\n"), RHS_list_of_columns ? "true" : "false");
   if (TYPEOF(values)==VECSXP && length(cols)==1 && length(values)==1) {

diff --git a/src/data.table.h b/src/data.table.h
@@ -118,6 +118,7 @@ extern SEXP sym_tzone;
 extern SEXP sym_old_fread_datetime_character;
 extern SEXP sym_variable_table;
 extern SEXP sym_as_character;
+extern SEXP sym_as_posixct;
 extern double NA_INT64_D;
 extern long long NA_INT64_LL;
 extern Rcomplex NA_CPLX;  // initialized in init.c; see there for comments

diff --git a/src/init.c b/src/init.c
@@ -42,6 +42,7 @@ SEXP sym_tzone;
 SEXP sym_old_fread_datetime_character;
 SEXP sym_variable_table;
 SEXP sym_as_character;
+SEXP sym_as_posixct;
 double NA_INT64_D;
 long long NA_INT64_LL;
 Rcomplex NA_CPLX;
@@ -297,6 +298,7 @@ void attribute_visible R_init_data_table(DllInfo *info)
   sym_old_fread_datetime_character = install("datatable.old.fread.datetime.character");
   sym_variable_table = install("variable_table");
   sym_as_character = install("as.character");
+  sym_as_posixct = install("as.POSIXct");
 
   initDTthreads();
   avoid_openmp_hang_within_fork();