Reset isReal test per-column to fix forder() with na.last=TRUE (#7877)

* set isReal per-column to fix na.last handling

* More tests of other data types

* NEWS entry

Author: ben-schwen

NEWS.md

@@ -80,6 +80,8 @@
 
 18. `example(local=TRUE)` where the example uses `[.data.table` works again (e.g. `example(':=', package='data.table', local=TRUE, echo=FALSE)`), [#7855](https://github.com/Rdatatable/data.table/issues/7855) re-fixing [#2972](https://github.com/Rdatatable/data.table/issues/2972). Thanks @michaelChirico for the fix.
 
+19. `DT[order(double, ..., -non_double, na.last=TRUE)]`, i.e., a double/complex column (in any order) followed by a non-double column in descending order with `na.last=TRUE`, is fixed to respect `na.last` again, [#7875](https://github.com/Rdatatable/data.table/issues/7875). The problematic behavior only occurred under specific conditions on the cardinality of the non-double column.
+
 ### Notes
 
 1. {data.table} now depends on R 3.5.0 (2018).

inst/tests/tests.Rraw

@@ -21891,3 +21891,24 @@ DF = DF[, 'a', drop=FALSE]
 test(2382.08, subset(DT, a > 5, select="a", drop=TRUE), subset(DF, a > 5, select="a", drop=TRUE))
 test(2382.09, subset(DT, a > 10, select="a", drop=TRUE), subset(DF, a > 10, select="a", drop=TRUE))
 test(2382.10, subset(DT, a > 5, drop=TRUE), subset(DF, a > 5, drop=TRUE))
+
+# forder() handles na.last=TRUE correctly in ordering double with >1 unique value then non-double (#7875)
+DT = data.table(
+  d0=c(10, 10, 10, 1),
+  l1=c(FALSE, TRUE, NA, TRUE),
+  i2=c(2:3, NA, 1L),
+  d3=c(2, 3, NA, 1),
+  z4=c(2+1i, 3+1i, NA, 1+1i),
+  c5=c('b', 'c', NA, 'a')
+)
+DT_sorted = DT[c(4L, 2:1, 3L)]
+test(2383.1, DT[order(d0, -l1, na.last=TRUE)], DT_sorted)
+test(2383.2, DT[order(d0, -i2, na.last=TRUE)], DT_sorted)
+test(2383.3, DT[order(d0, -d3, na.last=TRUE)], DT_sorted)
+test(2383.4, DT[order(d0, -z4, na.last=TRUE)], DT_sorted)
+test(2383.5, DT[order(d0, -c5, na.last=TRUE)], DT_sorted)
+if (test_bit64) {
+  DT[, i64 := as.integer64(i2)]
+  DT_sorted[, i64 := as.integer64(i2)]
+  test(2383.6, DT[order(d0, -i64, na.last=TRUE)], DT_sorted)
+}

src/forder.c

@@ -562,7 +562,6 @@ SEXP forder(SEXP DT, SEXP by, SEXP retGrpArg, SEXP retStatsArg, SEXP sortGroupsA
     STOP(_("Unable to allocate %"PRIu64" bytes of working memory"), (uint64_t)keyAlloc*sizeof(*key));  // # nocov
   nradix=0; // the current byte we're writing this column to; might be squashing into it (spare>0)
   int spare=0;  // the amount of bits remaining on the right of the current nradix byte
-  bool isReal=false;
   bool complexRerun = false;   // see comments below in CPLXSXP case
   SEXP CplxPart = R_NilValue;
   if (n_cplx) { CplxPart=PROTECT(allocVector(REALSXP, nrow)); n_protect++; } // one alloc is reused for each part
@@ -581,6 +580,7 @@ SEXP forder(SEXP DT, SEXP by, SEXP retGrpArg, SEXP retStatsArg, SEXP sortGroupsA
     }
     //Rprintf(_("sortType = %d\n"), sortType);
     hashtab * marks = NULL; // only used for STRSXP below
+    bool isReal=false;
     switch(TYPEOF(x)) {
     case INTSXP : case LGLSXP :  // TODO skip LGL and assume range [0,1]
       range_i32(INTEGER(x), nrow, &min, &max, &na_count);