Subset by bug (#2748)

inst/tests/tests.Rraw

@@ -5878,7 +5878,8 @@ DT <- data.table(intCol    = sample(c(1:3, NA), n, replace = TRUE),
                  doubleCol = sample(c(1.86, 1000.345, 2.346, NA, NaN), n, replace = TRUE),
                  boolCol   = sample(c(TRUE, FALSE, NA), n, replace = TRUE),
                  charCol   = sample(c(LETTERS[1:3], NA), n, replace = TRUE),
-                 groupCol  = sample(c("a", "b", "c"), n, replace = TRUE))
+                 groupCol  = sample(c("a", "b", "c"), n, replace = TRUE),
+                 sortedGroupCol = c(rep(1L, floor(n/3)), rep(2L, floor(n/3)), rep(3L, ceiling(n/3)))) ## sorted grouping column is important to test #2713
 if (test_bit64) DT[, int64Col := as.integer64(sample(1:3, n, replace = TRUE))]
 ## get list with unique values excluding NA
 vals <- lapply(DT, function(x) {out <- unique(x); out[!is.na(out)]})
@@ -5915,7 +5916,7 @@ all[, query := gsub("&missing$", "", gsub("missing&", "", query))]
 jQueries <- c(".(test = intCol + doubleCol, test2 = paste0(boolCol, charCol))",
               "c('test1', 'test2') := list(pmax(intCol, doubleCol), !boolCol)")
 ## define example 'by' values
-bys <- c("groupCol", character(0))
+bys <- c("groupCol", "sortedGroupCol", character(0))
 ## test each query string
 test_no <- 1437.0039
 for(t in seq_len(nrow(all))){

src/dogroups.c

@@ -132,10 +132,10 @@ SEXP dogroups(SEXP dt, SEXP dtcols, SEXP groups, SEXP grpcols, SEXP jiscols, SEX
              (char *)DATAPTR(VECTOR_ELT(groups,INTEGER(jiscols)[j]-1))+i*size,
              size);
     }
-    if (LOGICAL(on)[0])
-      igrp = (length(grporder) && isNull(jiscols)) ? INTEGER(grporder)[INTEGER(starts)[i]-1]-1 : i;
-    else
-      igrp = (length(grporder)) ? INTEGER(grporder)[INTEGER(starts)[i]-1]-1 : (isNull(jiscols) ? INTEGER(starts)[i]-1 : i);
+    // igrp determines the start of the current group in rows of dt (0 based).
+    // if jiscols is not null, we have a by = .EACHI, so the start is exactly i.
+    // Otherwise, igrp needs to be determined from starts, potentially taking care about the order if present.
+    igrp = !isNull(jiscols) ? i : (length(grporder) ? INTEGER(grporder)[INTEGER(starts)[i]-1]-1 : INTEGER(starts)[i]-1);
     if (igrp>=0 && nrowgroups) for (j=0; j<length(BY); j++) {    // igrp can be -1 so 'if' is important, otherwise memcpy crash
       size = SIZEOF(VECTOR_ELT(BY,j));
       memcpy((char *)DATAPTR(VECTOR_ELT(BY,j)),  // ok use of memcpy size 1. Loop'd through columns not rows