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.

Sign up for GitHub

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

extend checks for UTC inputs for robustness #4117

Merged
merged 7 commits into from
Dec 17, 2019
Merged

extend checks for UTC inputs for robustness #4117

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
39e04ee
extend checks for UTC inputs for robustness
Dec 16, 2019
e601148
tracing
Dec 16, 2019
e960313
revert
Dec 16, 2019
05e7485
tracing #2
Dec 16, 2019
a481959
more tracing
Dec 16, 2019
ac24a2e
maybe fixed?
Dec 16, 2019
1f3666f
test 1765 updated to pass #4088 when not testing under UTC; added new…
mattdowle Dec 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions R/IDateTime.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,10 @@ as.IDate.Date = function(x, ...) {
}

as.IDate.POSIXct = function(x, tz = attr(x, "tzone", exact=TRUE), ...) {
if (is.null(tz)) tz=''
if (tz %chin% c("UTC", "GMT")) {
if (is_utc(tz)) {
(setattr(as.integer(as.numeric(x) %/% 86400L), "class", c("IDate", "Date"))) # %/% returns new object so can use setattr() on it; wrap with () to return visibly
} else
as.IDate(as.Date(x, tz = tz, ...))
as.IDate(as.Date(x, tz = if (is.null(tz)) tz='' else tz, ...))
}

as.IDate.IDate = function(x, ...) x
Expand Down Expand Up @@ -138,9 +137,8 @@ as.ITime.default = function(x, ...) {
}

as.ITime.POSIXct = function(x, tz = attr(x, "tzone", exact=TRUE), ...) {
if (is.null(tz)) tz=''
if (tz %chin% c("UTC", "GMT")) as.ITime(unclass(x), ...)
else as.ITime(as.POSIXlt(x, tz = tz, ...), ...)
if (is_utc(tz)) as.ITime(unclass(x), ...)
else as.ITime(as.POSIXlt(x, tz = if (is.null(tz)) '' else tz, ...), ...)
}

as.ITime.numeric = function(x, ms = 'truncate', ...) {
Expand Down Expand Up @@ -305,19 +303,19 @@ as.POSIXlt.ITime = function(x, ...) {

second = function(x) {
# if we know the object is in UTC, can calculate the hour much faster
if (inherits(x, 'POSIXct') && identical(attr(x, 'tzone', exact=TRUE), 'UTC')) return(as.integer(as.numeric(x) %% 60L))
if (inherits(x, 'POSIXct') && is_utc(attr(x, 'tzone', exact=TRUE))) return(as.integer(as.numeric(x) %% 60L))
if (inherits(x, 'ITime')) return(as.integer(x) %% 60L)
as.integer(as.POSIXlt(x)$sec)
}
minute = function(x) {
# ever-so-slightly faster than x %% 3600L %/% 60L
if (inherits(x, 'POSIXct') && identical(attr(x, 'tzone', exact=TRUE), 'UTC')) return(as.integer(as.numeric(x) %/% 60L %% 60L))
if (inherits(x, 'POSIXct') && is_utc(attr(x, 'tzone', exact=TRUE))) return(as.integer(as.numeric(x) %/% 60L %% 60L))
if (inherits(x, 'ITime')) return(as.integer(x) %/% 60L %% 60L)
as.POSIXlt(x)$min
}
hour = function(x) {
# ever-so-slightly faster than x %% 86400L %/% 3600L
if (inherits(x, 'POSIXct') && identical(attr(x, 'tzone', exact=TRUE), 'UTC')) return(as.integer(as.numeric(x) %/% 3600L %% 24L))
if (inherits(x, 'POSIXct') && is_utc(attr(x, 'tzone', exact=TRUE))) return(as.integer(as.numeric(x) %/% 3600L %% 24L))
if (inherits(x, 'ITime')) return(as.integer(x) %/% 3600L %% 24L)
as.POSIXlt(x)$hour
}
Expand Down
8 changes: 8 additions & 0 deletions R/utils.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,14 @@ do_patterns = function(pat_sub, all_cols) {
return(matched)
}

# check UTC status
is_utc = function(tz) {
# via grep('UTC|GMT', OlsonNames(), value = TRUE)
utc_tz = c("Etc/GMT", "Etc/UTC", "GMT", "GMT-0", "GMT+0", "GMT0", "UTC")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sorted this alphabetically... perhaps we should sort it by expected frequency instead? Does %chin% have a short-out to return once the first match is found?

Copy link
Member

@mattdowle mattdowle Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Good idea. If its LHS is length 1, then it should just loop through the RHS and stop early if and when it is found. Will do. (Btw, sorting the input was good thought but doesn't make any difference as it's an order-n two-pass approach using the now-well- established truelength clobber technique.)

Copy link
Member

@mattdowle mattdowle Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When %chin% does short-circuit as you suggested, then yes this input should be sorted by expected frequency. Good idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: #4121

if (is.null(tz)) tz = Sys.timezone()
return(tz %chin% utc_tz)
}

# nocov start #593 always return a data.table
edit.data.table = function(name, ...) {
setDT(NextMethod('edit', name))[]
Expand Down