Skip to content

move argument validation from [ #7937

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

Open
ben-schwen wants to merge 1 commit into from
+35 −24
Open

move argument validation from [ #7937

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
59 changes: 35 additions & 24 deletions R/data.table.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,36 @@ replace_dot_alias = function(e) {
ans
}

# validate and normalise the arguments of [.data.table that don't depend on i, j or by. Returns
# list(roll=, rollends=, nomatch=, nomatch0=).
.dt_check_args = function(mult, roll, rollends, nomatch, which, with, showProgress, missing_j) {
if (!mult %chin% c("first", "last", "all", "error")) stopf("mult argument can only be 'first', 'last', 'all' or 'error'")
if (length(roll)!=1L || is.na(roll)) stopf("roll must be a single TRUE, FALSE, positive/negative integer/double including +Inf and -Inf or 'nearest'")
if (is.character(roll)) {
if (roll!="nearest") stopf("roll is '%s' (type character). Only valid character value is 'nearest'.", roll)
} else {
roll = if (isTRUE(roll)) +Inf else as.double(roll)
}
Comment on lines +547 to +551

@MichaelChirico MichaelChirico Oct 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Possibly nicer?

Suggested change
if (is.character(roll)) {
if (roll!="nearest") stopf("roll is '%s' (type character). Only valid character value is 'nearest'.", roll)
} else {
roll = if (isTRUE(roll)) +Inf else as.double(roll)
}
if (!is.character(roll)) {
roll = if (isTRUE(roll)) +Inf else as.double(roll)
} else if (roll != "nearest") {
stopf("roll is '%s' (type character). Only valid character value is 'nearest'.", roll)
}

Or even

Suggested change
if (is.character(roll)) {
if (roll!="nearest") stopf("roll is '%s' (type character). Only valid character value is 'nearest'.", roll)
} else {
roll = if (isTRUE(roll)) +Inf else as.double(roll)
}
if (is.numeric(roll)) {
roll = as.double(roll)
} else if (isTRUE(roll)) {
roll = +Inf
} else if (roll != "nearest") {
stopf("roll is '%s' (type character). Only valid character value is 'nearest'.", roll)
}

With the caveat that the error branch now also picks up non-numeric roll that was previously accepted and coerced (e.g. roll = 1+1i)

force(rollends)
if (!is.logical(rollends)) stopf("rollends must be a logical vector")
if (length(rollends)>2L) stopf("rollends must be length 1 or 2")
if (length(rollends)==1L) rollends=rep.int(rollends,2L)
# TO DO (document/faq/example). Removed for now ... if ((roll || rolltolast) && missing(mult)) mult="last" # for when there is exact match to mult. This does not control cases where the roll is mult, that is always the last one.
.unsafe.opt() #3585
nomatch0 = identical(nomatch,0.0) || identical(nomatch,0L) || identical(nomatch, FALSE) # for warning with row-numbers in i; #4353
if (nomatch0) nomatch=NULL # retain nomatch=0|FALSE backwards compatibility, #857 #5214
if (!is.null(nomatch)) {
if (!(length(nomatch)==1L && is.na(nomatch))) stopf("nomatch= must be either NA or NULL (or 0 for backwards compatibility which is the same as NULL but please use NULL)")
nomatch=NA # convert NA_character_ to NA-logical, PR#5216
}
if (!is.logical(which) || length(which)>1L) stopf("which= must be a logical vector length 1. Either FALSE, TRUE or NA.")
if ((isTRUE(which)||is.na(which)) && !missing_j) stopf("which==%s (meaning return row numbers) but j is also supplied. Either you need row numbers or the result of j, but only one type of result can be returned.", which)

@MichaelChirico MichaelChirico Oct 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

similarly to #7936, I wonder about just re-computing missing(j) here. On this first branch, it's clearly the unusual code path; on the latter one, it's more "typical". Also, this helper has a ton of arguments anyway, so one extra one is not so bad.

if (is.null(nomatch) && is.na(which)) stopf("which=NA with nomatch=0|NULL would always return an empty vector. Please change or remove either which or nomatch.")
if (!with && missing_j) stopf("j must be provided when with=FALSE")
if (!(isTRUEorFALSE(showProgress) || (is.numeric(showProgress) && length(showProgress)==1L && showProgress >= 0))) stopf("showProgress must be TRUE, FALSE, or a single non-negative number") # nocov
list(roll=roll, rollends=rollends, nomatch=nomatch, nomatch0=nomatch0)
}

"[.data.table" = function(x, i, j, by, keyby, with=TRUE, nomatch=NA, mult="all", roll=FALSE, rollends=if (roll=="nearest") c(TRUE,TRUE) else if (roll>=0.0) c(FALSE,TRUE) else c(TRUE,FALSE), which=FALSE, .SDcols, verbose=getOption("datatable.verbose"), allow.cartesian=getOption("datatable.allow.cartesian"), drop=NULL, on=NULL, env=NULL, showProgress=getOption("datatable.showProgress", interactive()))
{
# ..selfcount <<- ..selfcount+1 # in dev, we check no self calls, each of which doubles overhead, or could
Expand Down Expand Up @@ -613,30 +643,11 @@ replace_dot_alias = function(e) {
}
return(x)
}
if (!mult %chin% c("first", "last", "all", "error")) stopf("mult argument can only be 'first', 'last', 'all' or 'error'")
if (length(roll)!=1L || is.na(roll)) stopf("roll must be a single TRUE, FALSE, positive/negative integer/double including +Inf and -Inf or 'nearest'")
if (is.character(roll)) {
if (roll!="nearest") stopf("roll is '%s' (type character). Only valid character value is 'nearest'.", roll)
} else {
roll = if (isTRUE(roll)) +Inf else as.double(roll)
}
force(rollends)
if (!is.logical(rollends)) stopf("rollends must be a logical vector")
if (length(rollends)>2L) stopf("rollends must be length 1 or 2")
if (length(rollends)==1L) rollends=rep.int(rollends,2L)
# TO DO (document/faq/example). Removed for now ... if ((roll || rolltolast) && missing(mult)) mult="last" # for when there is exact match to mult. This does not control cases where the roll is mult, that is always the last one.
.unsafe.opt() #3585
nomatch0 = identical(nomatch,0.0) || identical(nomatch,0L) || identical(nomatch, FALSE) # for warning with row-numbers in i; #4353
if (nomatch0) nomatch=NULL # retain nomatch=0|FALSE backwards compatibility, #857 #5214
if (!is.null(nomatch)) {
if (!(length(nomatch)==1L && is.na(nomatch))) stopf("nomatch= must be either NA or NULL (or 0 for backwards compatibility which is the same as NULL but please use NULL)")
nomatch=NA # convert NA_character_ to NA-logical, PR#5216
}
if (!is.logical(which) || length(which)>1L) stopf("which= must be a logical vector length 1. Either FALSE, TRUE or NA.")
if ((isTRUE(which)||is.na(which)) && !missing_j) stopf("which==%s (meaning return row numbers) but j is also supplied. Either you need row numbers or the result of j, but only one type of result can be returned.", which)
if (is.null(nomatch) && is.na(which)) stopf("which=NA with nomatch=0|NULL would always return an empty vector. Please change or remove either which or nomatch.")
if (!with && missing_j) stopf("j must be provided when with=FALSE")
if (!(isTRUEorFALSE(showProgress) || (is.numeric(showProgress) && length(showProgress)==1L && showProgress >= 0))) stopf("showProgress must be TRUE, FALSE, or a single non-negative number") # nocov
args_ok = .dt_check_args(mult, roll, rollends, nomatch, which, with, showProgress, missing_j)
roll = args_ok$roll
rollends = args_ok$rollends
nomatch = args_ok$nomatch
nomatch0 = args_ok$nomatch0
irows = NULL # Meaning all rows. We avoid creating 1:nrow(x) for efficiency.
notjoin = FALSE
rightcols = leftcols = integer()
Expand Down
Loading