Both columns for rolling and non-equi joins

[mattdowle]

I can't push to remote branch sritchie73:non-equi-key so copied it into main from PR #2706.
Looks like commit history from Scott is retained which is good, and retained the branch name.
The extended discussion history will just be in the previous PR #2706 though. Can't think of a way round this. Scott is member of project so can create branches in main going forward.

Closes #1615
Closes #1700
Closes #2006
Closes #2569
Other related issues already closed (e.g. dups) : #1469, #1807, #2307, #2595, #2602

• test.data.table() fails with 28 errors when datatable.bothkeycols is set to TRUE (caused by typo datatable.bothkeycol vs datatable.bothkeycols)
• bothkeycols= renamed to old.nonequi= (explanation in comments below)
• Place extra roll= column just after the first one, not at the end of the result.
• Add short examples to the news item

[jangorecki]

@sritchie73 could you please merge to recent master and work out code coverage?

[tdhock]

hi any update/plan to merge this soon? I would love to use non-equi join more often in research and teaching and this PR would make it so much easier to use/explain.

[tdhock]

I'm not sure how easy this will be to get work after all this time, but I think it is an important feature, so I tried to resolve the conflicts with master.

[tdhock]

on master we have in data.table.R

      ans = bmerge(i, x, leftcols, rightcols, roll, rollends, nomatch, mult, ops, verbose=verbose)

whereas that line in this branch is now

      ans = bmerge(i, x, leftcols, rightcols, xo, roll, rollends, nomatch, mult, ops, nqgrp, nqmaxgrp, verbose=verbose)

https://github.com/Rdatatable/data.table/blame/c6e5a0384ce2d6fe5f45406ffc07baea9ab2d460/R/data.table.R#L513 shows this line was modified in this pr, but why is the definition of bmerge not consistently updated in this pr?
I guess master removed some arguments in bmerge, relative to when this pr started.
there were conflicts on the line above (usage of bmerge in data.table.R), but not in definition of bmerge in bmerge.R.

currently bmerge.R is defined as below so I guess the solution is to remove the extra arguments.

bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbose)

[github-actions]

Generated via commit 7889c86

Download link for the artifact containing the test results: ↓ atime-results.zip

Task	Duration
R setup and installing dependencies	4 minutes and 43 seconds
Installing different package versions	7 minutes and 35 seconds
Running and plotting the test cases	2 minutes and 11 seconds

[tdhock]

we are getting non_equi not found error now.
https://github.com/Rdatatable/data.table/blame/ded8afba061cf016064d6efaa26483c72f9aca06/R/data.table.R#L502 says that non_equi was present before trying to merge with master. it was previously touched in #2420 (Oct 2017)
if(... is.na(non_equi) ) is deleted on master, but used in this pr.
the merge conflict diff shows that, but not the definition of non_equi.
i guess we have to dig in the previous version of the code before I attempted the merge, https://github.com/Rdatatable/data.table/blob/ded8afba061cf016064d6efaa26483c72f9aca06/R/data.table.R
sorry for the messy merge conflict.
this is the first merge conflict in which I have had to dig for contexual code outside the <<< === >>> merge conflict diff block (which usually is sufficient in my experience).
I computed a diff between the two versions of the code which seem to be analogous

diff -u "c:/Users/hoct2726/R/data.table/R/data.table-master.R" "c:/Users/hoct2726/R/data.table/R/data.table-before-merge.R"
--- c:/Users/hoct2726/R/data.table/R/data.table-master.R	2024-11-28 14:37:50.101759300 -0500
+++ c:/Users/hoct2726/R/data.table/R/data.table-before-merge.R	2024-11-28 14:37:47.261831400 -0500
@@ -1,43 +1,78 @@
-    if (is.data.frame(i)) {
-      if (missing(on)) {
-        if (!haskey(x)) {
-          stopf("When i is a data.table (or character vector), the columns to join by must be specified using 'on=' argument (see ?data.table), by keying x (i.e. sorted, and, marked as sorted, see ?setkey), or by sharing column names between x and i (i.e., a natural join). Keyed joins might have further speed benefits on very large data due to x being sorted in RAM.")
-        }
-      } else if (identical(substitute(on), as.name(".NATURAL"))) {
-        naturaljoin = TRUE
-      }
-      if (naturaljoin) { # natural join #629
-        common_names = intersect(names_x, names(i))
-        len_common_names = length(common_names)
-        if (!len_common_names) stopf("Attempting to do natural join but no common columns in provided tables")
-        if (verbose) {
-          which_cols_msg = if (len_common_names == length(x)) {
-            catf("Joining but 'x' has no key, natural join using all 'x' columns")
-          } else {
-            catf("Joining but 'x' has no key, natural join using: %s", brackify(common_names))
-          }
-        }
-        on = common_names
+    if (is.data.table(i)) {
+      if (!haskey(x) && missing(on) && is.null(xo)) {
+        stop("When i is a data.table (or character vector), the columns to join by must be specified either using 'on=' argument (see ?data.table) or by keying x (i.e. sorted, and, marked as sorted, see ?setkey). Keyed joins might have further speed benefits on very large data due to x being sorted in RAM.")
       }
       if (!missing(on)) {
         # on = .() is now possible, #1257
         on_ops = .parse_on(substitute(on), isnull_inames)
         on = on_ops[[1L]]
         ops = on_ops[[2L]]
-        if (any(ops > 1L)) { ## fix for #4489;  ops = c("==", "<=", "<", ">=", ">", "!=")
-          allow.cartesian = TRUE
-        }
         # TODO: collect all '==' ops first to speeden up Cnestedid
-        rightcols = colnamesInt(x, names(on), check_dups=FALSE)
-        leftcols  = colnamesInt(i, unname(on), check_dups=FALSE)
-      } else {
-        ## missing on
-        rightcols = chmatch(key(x), names_x)   # NAs here (i.e. invalid data.table) checked in bmerge()
+        rightcols = chmatch(names(on), names(x))
+        if (length(nacols <- which(is.na(rightcols))))
+          stop("Column(s) [", paste(names(on)[nacols], collapse=","), "] not found in x")
+        leftcols  = chmatch(unname(on), names(i))
+        if (length(nacols <- which(is.na(leftcols))))
+          stop("Column(s) [", paste(unname(on)[nacols], collapse=","), "] not found in i")
+        # figure out the columns on which to compute groups on
+        non_equi = which.first(ops != 1L) # 1 is "==" operator
+        if (!is.na(non_equi)) {
+          # non-equi operators present.. investigate groups..
+          if (verbose) cat("Non-equi join operators detected ... \n")
+          if (!missingroll) stop("roll is not implemented for non-equi joins yet.")
+          if (verbose) {last.started.at=proc.time();cat("  forder took ... ");flush.console()}
+          # TODO: could check/reuse secondary indices, but we need 'starts' attribute as well!
+          xo = forderv(x, rightcols, retGrp=TRUE)
+          if (verbose) {cat(timetaken(last.started.at),"\n"); flush.console()}
+          xg = attr(xo, 'starts')
+          resetcols = head(rightcols, non_equi-1L)
+          if (length(resetcols)) {
+            # TODO: can we get around having to reorder twice here?
+            # or at least reuse previous order?
+            if (verbose) {last.started.at=proc.time();cat("  Generating group lengths ... ");flush.console()}
+            resetlen = attr(forderv(x, resetcols, retGrp=TRUE), 'starts')
+            resetlen = .Call(Cuniqlengths, resetlen, nrow(x))
+            if (verbose) {cat("done in",timetaken(last.started.at),"\n"); flush.console()}
+          } else resetlen = integer(0L)
+          if (verbose) {last.started.at=proc.time();cat("  Generating non-equi group ids ... ");flush.console()}
+          nqgrp = .Call(Cnestedid, x, rightcols[non_equi:length(rightcols)], xo, xg, resetlen, mult)
+          if (verbose) {cat("done in",timetaken(last.started.at),"\n"); flush.console()}
+          if (length(nqgrp)) nqmaxgrp = max(nqgrp) # fix for #1986, when 'x' is 0-row table max(.) returns -Inf.
+          if (nqmaxgrp > 1L) { # got some non-equi join work to do
+            if ("_nqgrp_" %in% names(x)) stop("Column name '_nqgrp_' is reserved for non-equi joins.")
+            if (verbose) {last.started.at=proc.time();cat("  Recomputing forder with non-equi ids ... ");flush.console()}
+            set(nqx<-shallow(x), j="_nqgrp_", value=nqgrp)
+            xo = forderv(nqx, c(ncol(nqx), rightcols))
+            if (verbose) {cat("done in",timetaken(last.started.at),"\n"); flush.console()}
+          } else nqgrp = integer(0L)
+          if (verbose) cat("  Found", nqmaxgrp, "non-equi group(s) ...\n")
+        }
+        if (is.na(non_equi)) {
+          # equi join. use existing key (#1825) or existing secondary index (#1439)
+          if ( identical(head(key(x), length(on)), names(on)) ) {
+            xo = integer(0L)
+            if (verbose) cat("on= matches existing key, using key\n")
+          } else {
+            if (isTRUE(getOption("datatable.use.index"))) {
+              xo = getindex(x, names(on))
+              if (verbose && !is.null(xo)) cat("on= matches existing index, using index\n")
+            }
+            if (is.null(xo)) {
+              if (verbose) {last.started.at=proc.time(); flush.console()}
+              xo = forderv(x, by = rightcols)
+              if (verbose) {cat("Calculated ad hoc index in",timetaken(last.started.at),"\n"); flush.console()}
+              # TODO: use setindex() instead, so it's cached for future reuse
+            }
+          }
+        }
+      } else if (is.null(xo)) {
+        rightcols = chmatch(key(x),names(x))   # NAs here (i.e. invalid data.table) checked in bmerge()
         leftcols = if (haskey(i))
-          chmatch(head(key(i), length(rightcols)), names(i))
+          chmatch(head(key(i),length(rightcols)),names(i))
         else
           seq_len(min(length(i),length(rightcols)))
         rightcols = head(rightcols,length(leftcols))
+        xo = integer()  ## signifies 1:.N
         ops = rep(1L, length(leftcols))
       }
       # Implementation for not-join along with by=.EACHI, #604

I could use some help from anyone who has hacked data.table.R (this is my first time).

[jangorecki]

I would rather look at the diff of this PR vs master at the time of fork, then understand the logic, then apply same logic in a fresh branch forked from current master.

[sritchie73]

👍 to @jangorecki 's suggestion - from recollection at the time it was also one of my first attempts at modifying the data.table package, so the diff between the original upstream commit and the changes made should be relatively small. You may then want to create a new clean branch from the current master and incorporate that same logic as needed, rather than modifying my very old branch.