Fixes for integer64 issues in between (#7193)

* tests: failing integer64 readable as small integer

* between: check for integer64 before coercion

In particular, avoid calling fitsInInt32() on REALSXP vectors that could
possibly be of class 'integer64' (and therefore shouldn't be read as
doubles).

* fitInInt*: disallow integer64

Since the functions themselves read the vector as double* and are used
as a test whether doubles from the vector would fit in an integer, avoid
reading integer64 vectors altogether.

* tests: failing blind double -> int64 conversion

* between: convert bounds to int64 only if they fit

Avoid auto-converting 'lower' or 'upper' to integer64 if they aren't
actually representable in int64_t.

* Apply suggestions from code review

Unify the translatable strings.

Co-authored-by: Michael Chirico <[email protected]>

* Fix tests for error messages

* Make the error messages more symmetric.

Co-authored-by: Michael Chirico <[email protected]>

---------

Co-authored-by: Michael Chirico <[email protected]>

Author: aitap

R/between.R

@@ -30,8 +30,8 @@ between = function(x, lower, upper, incbounds=TRUE, NAbounds=TRUE, check=FALSE,
   }
   if (is.i64(x)) {
     if (!requireNamespace("bit64", quietly=TRUE)) stopf("trying to use integer64 class when 'bit64' package is not installed") # nocov
-    if (!is.i64(lower) && is.numeric(lower)) lower = bit64::as.integer64(lower)
-    if (!is.i64(upper) && is.numeric(upper)) upper = bit64::as.integer64(upper)
+    if (!is.i64(lower) && (is.integer(lower) || fitsInInt64(lower))) lower = bit64::as.integer64(lower)
+    if (!is.i64(upper) && (is.integer(upper) || fitsInInt64(upper))) upper = bit64::as.integer64(upper)
   }
   is.supported = function(x) is.numeric(x) || is.character(x) || is.px(x)
   if (is.supported(x) && is.supported(lower) && is.supported(upper)) {

inst/tests/tests.Rraw

@@ -15068,7 +15068,7 @@ if (test_bit64) {
   as.i64 = bit64::as.integer64
   test(2039.01, between(1:10, as.i64(3), as.i64(6)),  error="x is not integer64 but.*Please align classes")
   test(2039.02, between(1:10, 3, as.i64(6)),          error="x is not integer64 but.*Please align classes")
-  test(2039.03, between(as.i64(1:3), "2", as.i64(4)), error="x is integer64 but lower and/or upper are not")
+  test(2039.03, between(as.i64(1:3), "2", as.i64(4)), error="x is integer64 but lower is not.*Please align classes")
   old = options("datatable.verbose"=TRUE)
   x = as.i64(1:10)
   ans36 = c(FALSE,FALSE,TRUE,TRUE,TRUE,TRUE,FALSE,FALSE,FALSE,FALSE)
@@ -15095,6 +15095,10 @@ if (test_bit64) {
   test(2039.19, between(x+maxint, 3+maxint, NA, incbounds=FALSE), c(head(ans36open, -5L), rep(TRUE, 5)), output="between parallel processing of integer64 took")
   test(2039.20, between(x+maxint, rep(NA, 10L), rep(6+maxint, 10L)), c(TRUE, TRUE, tail(ans36, -2L)), output="between parallel processing of integer64 took")
   test(2039.21, between(x+maxint, rep(3+maxint, 10L), rep(NA, 10L), incbounds=FALSE), c(head(ans36open, -5L), rep(TRUE, 5)), output="between parallel processing of integer64 took")
+  # must not blindly read integer64 values as doubles when the latter fit into int32, #7164
+  test(2039.22, between(42L, structure(41., class="integer64"), structure(43., class="integer64")), error="x is not integer64 but.*Please align classes")
+  # must not blindly convert numeric bounds to integer64, #7164
+  test(2039.23, between(as.i64(42), 41, -2^98), error="x is integer64 but upper is not.*Please align classes")
   options(old)
 }
 

src/between.c

@@ -29,6 +29,19 @@ SEXP between(SEXP x, SEXP lower, SEXP upper, SEXP incbounds, SEXP NAboundsArg, S
   const bool check = LOGICAL(checkArg)[0];
   const bool verbose = GetVerbose();
 
+  // check before potential coercion which ignores methods, #7164
+  if (INHERITS(x, char_integer64)) {
+    if (!INHERITS(lower, char_integer64))
+      error(_("x is integer64 but %s is not. Please align classes."), "lower"); // e.g. between(int64, character, character)
+    if (!INHERITS(upper, char_integer64))
+      error(_("x is integer64 but %s is not. Please align classes."), "upper"); // e.g. between(int64, character, character)
+  } else {
+    if (INHERITS(lower, char_integer64))
+      error(_("x is not integer64 but %s is. Please align classes."), "lower");
+    if (INHERITS(upper, char_integer64))
+      error(_("x is not integer64 but %s is. Please align classes."), "upper");
+  }
+
   if (isInteger(x)) {
     if ((isInteger(lower) || fitsInInt32(lower)) &&
         (isInteger(upper) || fitsInInt32(upper))) { // #3517 coerce to num to int when possible
@@ -90,8 +103,6 @@ SEXP between(SEXP x, SEXP lower, SEXP upper, SEXP incbounds, SEXP NAboundsArg, S
 
   case REALSXP:
     if (INHERITS(x, char_integer64)) {
-      if (!INHERITS(lower, char_integer64) || !INHERITS(upper, char_integer64))
-        error(_("x is integer64 but lower and/or upper are not.")); // e.g. between(int64, character, character)
       const int64_t *lp = (int64_t *)REAL(lower);
       const int64_t *up = (int64_t *)REAL(upper);
       const int64_t *xp = (int64_t *)REAL(x);
@@ -117,8 +128,6 @@ SEXP between(SEXP x, SEXP lower, SEXP upper, SEXP incbounds, SEXP NAboundsArg, S
       }
       if (verbose) Rprintf(_("between parallel processing of integer64 took %8.3fs\n"), omp_get_wtime()-tic);
     } else {
-      if (INHERITS(lower, char_integer64) || INHERITS(upper, char_integer64))
-        error(_("x is not integer64 but lower and/or upper is integer64. Please align classes."));
       const double *lp = REAL(lower);
       const double *up = REAL(upper);
       const double *xp = REAL(x);

src/utils.c

@@ -14,7 +14,7 @@ bool within_int64_repres(double x) {
 // used to error if not passed type double but this needed extra is.double() calls in calling R code
 // which needed a repeat of the argument. Hence simpler and more robust to return false when not type double.
 bool fitsInInt32(SEXP x) {
-  if (!isReal(x))
+  if (!isReal(x) || INHERITS(x, char_integer64))
     return false;
   R_xlen_t n=xlength(x), i=0;
   const double *dx = REAL(x);
@@ -31,7 +31,7 @@ SEXP fitsInInt32R(SEXP x) {
 }
 
 bool fitsInInt64(SEXP x) {
-  if (!isReal(x))
+  if (!isReal(x) || INHERITS(x, char_integer64))
     return false;
   R_xlen_t n=xlength(x), i=0;
   const double *dx = REAL(x);