automatically convert posixlt to posixct in set/:=/<- #6299
Conversation
ben-schwen
requested review from
HughParsonage and
MichaelChirico
as code owners
|
Generated via commit 013837f Download link for the artifact containing the test results: ↓ atime-results.zip Time taken to finish the standard R installation steps: 11 minutes and 32 seconds Time taken to run |
|
related to an issue I was having many years ago #2068 (comment) > data.table(x=strptime("2024-01-01","%Y-%m-%d"))
x
<POSc>
1: 2024-01-01
Warning message:
In as.data.table.list(x, keep.rownames = keep.rownames, check.names = check.names, :
POSIXlt column type detected and converted to POSIXct. We do not recommend use of POSIXlt at all because it uses 40 bytes to store one date.
> data.table()[, x := strptime("2024-01-01","%Y-%m-%d")]
Warning message:
In strptime("2024-01-01", "%Y-%m-%d") :
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.
> class(strptime("2024-01-01","%Y-%m-%d"))
[1] "POSIXlt" "POSIXt" So I think what you are doing is great, you may look at what is done in |
src/assign.c
Outdated
| @@ -428,6 +428,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 uses 40+ bytes to store one date. Use as.POSIXct() to avoid this warning.")); | |||
There was a problem hiding this comment.
compare with warning from data.table()
POSIXlt column type detected and converted to POSIXct. We do not recommend use of POSIXlt at all because it uses 40 bytes to store one date.
Can the two warnings be made exactly the same? (easier for translators probably)
There was a problem hiding this comment.
(easier for translators probably)
Possibly, but this is thrown from C while the other is thrown from R, so currently they live in different domains (R-data.table.pot vs data.table.pot), so a given translator might not even know about the two different messages. I think there are two ways to align them:
- Make the R message thrown from a C wrapper like
posixlt_warning = function() .Call(Cposixlt_warning) - Call
gettext(., domain="data.table")on the exact string used in C.
The former adds a little clutter to our .so, while the latter is a bit fragile by depending on the exact wording. I'm not sure either is better, or if either is worth investing in in the first place.
There was a problem hiding this comment.
Not sure the effort to align them is worth, if it is not ensured that both are translated by the same translator (or by the same translation string).
|
Marking this for 1.17.0 -- I suspect there can be downstream breakages as a result. Better to defer till after release. |
|
revdeps checked today and did not find anything, https://rcdata.nau.edu/genomic-ml/data.table-revdeps/analyze/2024-08-28/ |
|
Does it also close #4800? |
|
@iago-pssjd Nope since this only covers operations on |
Closes #1724