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

Check roll-on-factor up-front to simplify later checks #6639

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Check roll-on-factor up-front to simplify later checks #6639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28edd2b
small refactor of bmerge() to check roll-on-factor condition first
MichaelChirico Dec 7, 2024
b4a3e72
Merge remote-tracking branch 'origin/master' into bmerge-helper
MichaelChirico Dec 7, 2024
0158272
restore factor checks as coming first
MichaelChirico Dec 7, 2024
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
6 changes: 4 additions & 2 deletions R/between.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,10 @@ inrange = function(x,lower,upper,incbounds=TRUE) {
verbose = isTRUE(getOption("datatable.verbose"))
if (verbose) {last.started.at=proc.time();catf("forderv(query) took ... ");flush.console()}
if (verbose) {cat(timetaken(last.started.at),"\n"); flush.console()}
ans = bmerge(shallow(subject), query, 1L:2L, c(1L,1L),
0, c(FALSE, TRUE), 0L, "all", ops, verbose) # fix for #1819, turn on verbose messages
ans = bmerge(
shallow(subject), query,
icols=1L:2L, xcols=c(1L,1L),
roll=0.0, rollends=c(FALSE, TRUE), nomatch=0L, mult="all", ops, verbose) # fix for #1819, turn on verbose messages
xo = ans$xo
options(datatable.verbose=FALSE)
setDT(ans[c("starts", "lens")], key=c("starts", "lens"))
Expand Down
11 changes: 8 additions & 3 deletions R/bmerge.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@ coerce_col = function(dt, col, from_type, to_type, from_name, to_name, verbose_m

bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbose)
{
if (roll != 0.0 && length(icols)) {
last_x_idx = tail(xcols, 1L)
last_i_idx = tail(icols, 1L)
if (is.factor(x[[last_x_idx]]) || is.factor(i[[last_i_idx]]))
stopf("Attempting roll join on factor column when joining x.%s to i.%s. Only integer, double or character columns may be roll joined.", names(x)[last_x_idx], names(i)[last_i_idx])
}

callersi = i
i = shallow(i)
# Just before the call to bmerge() in [.data.table there is a shallow() copy of i to prevent coercions here
Expand Down Expand Up @@ -59,9 +66,8 @@ bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbos
iname = paste0("i.", names(i)[icol])
if (!x_merge_type %chin% supported) stopf("%s is type %s which is not supported by data.table join", xname, x_merge_type)
if (!i_merge_type %chin% supported) stopf("%s is type %s which is not supported by data.table join", iname, i_merge_type)
# we check factors first because they might have different levels
if (x_merge_type=="factor" || i_merge_type=="factor") {
if (roll!=0.0 && a==length(icols))
stopf("Attempting roll join on factor column when joining %s to %s. Only integer, double or character columns may be roll joined.", xname, iname)
if (x_merge_type=="factor" && i_merge_type=="factor") {
if (verbose) catf("Matching %s factor levels to %s factor levels.\n", iname, xname)
set(i, j=icol, value=chmatch(levels(i[[icol]]), levels(x[[xcol]]), nomatch=0L)[i[[icol]]]) # nomatch=0L otherwise a level that is missing would match to NA values
Expand All @@ -82,7 +88,6 @@ bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbos
}
stopf("Incompatible join types: %s (%s) and %s (%s). Factor columns must join to factor or character columns.", xname, x_merge_type, iname, i_merge_type)
}
# we check factors first to cater for the case when trying to do rolling joins on factors
if (x_merge_type == i_merge_type) {
if (verbose) catf("%s has same type (%s) as %s. No coercion needed.\n", iname, x_merge_type, xname)
next
Expand Down
Loading