FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fixes for `integer64` issues in `between` (#7193) · Rdatatable/data.table@52b1ceb · GitHub

/ data.table Public

Commit 52b1ceb

Browse files
aitap
and
MichaelChirico
authored
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 <chiricom@google.com> * Fix tests for error messages * Make the error messages more symmetric. Co-authored-by: Michael Chirico <chiricom@google.com> --------- Co-authored-by: Michael Chirico <chiricom@google.com>
1 parent 7f14f2e commit 52b1ceb

4 files changed

Lines changed: 22 additions & 9 deletions

File tree

‎R/between.R‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,8 @@ between = function(x, lower, upper, incbounds=TRUE, NAbounds=TRUE, check=FALSE,
3030
}
3131
if (is.i64(x)) {
3232
if (!requireNamespace("bit64", quietly=TRUE)) stopf("trying to use integer64 class when 'bit64' package is not installed") # nocov
33-
if (!is.i64(lower) && is.numeric(lower)) lower = bit64::as.integer64(lower)
34-
if (!is.i64(upper) && is.numeric(upper)) upper = bit64::as.integer64(upper)
33+
if (!is.i64(lower) && (is.integer(lower) || fitsInInt64(lower))) lower = bit64::as.integer64(lower)
34+
if (!is.i64(upper) && (is.integer(upper) || fitsInInt64(upper))) upper = bit64::as.integer64(upper)
3535
}
3636
is.supported = function(x) is.numeric(x) || is.character(x) || is.px(x)
3737
if (is.supported(x) && is.supported(lower) && is.supported(upper)) {

‎inst/tests/tests.Rraw‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15068,7 +15068,7 @@ if (test_bit64) {
1506815068
as.i64 = bit64::as.integer64
1506915069
test(2039.01, between(1:10, as.i64(3), as.i64(6)), error="x is not integer64 but.*Please align classes")
1507015070
test(2039.02, between(1:10, 3, as.i64(6)), error="x is not integer64 but.*Please align classes")
15071-
test(2039.03, between(as.i64(1:3), "2", as.i64(4)), error="x is integer64 but lower and/or upper are not")
15071+
test(2039.03, between(as.i64(1:3), "2", as.i64(4)), error="x is integer64 but lower is not.*Please align classes")
1507215072
old = options("datatable.verbose"=TRUE)
1507315073
x = as.i64(1:10)
1507415074
ans36 = c(FALSE,FALSE,TRUE,TRUE,TRUE,TRUE,FALSE,FALSE,FALSE,FALSE)
@@ -15095,6 +15095,10 @@ if (test_bit64) {
1509515095
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")
1509615096
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")
1509715097
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")
15098+
# must not blindly read integer64 values as doubles when the latter fit into int32, #7164
15099+
test(2039.22, between(42L, structure(41., class="integer64"), structure(43., class="integer64")), error="x is not integer64 but.*Please align classes")
15100+
# must not blindly convert numeric bounds to integer64, #7164
15101+
test(2039.23, between(as.i64(42), 41, -2^98), error="x is integer64 but upper is not.*Please align classes")
1509815102
options(old)
1509915103
}
1510015104

‎src/between.c‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,19 @@ SEXP between(SEXP x, SEXP lower, SEXP upper, SEXP incbounds, SEXP NAboundsArg, S
2929
const bool check = LOGICAL(checkArg)[0];
3030
const bool verbose = GetVerbose();
3131

32+
// check before potential coercion which ignores methods, #7164
33+
if (INHERITS(x, char_integer64)) {
34+
if (!INHERITS(lower, char_integer64))
35+
error(_("x is integer64 but %s is not. Please align classes."), "lower"); // e.g. between(int64, character, character)
36+
if (!INHERITS(upper, char_integer64))
37+
error(_("x is integer64 but %s is not. Please align classes."), "upper"); // e.g. between(int64, character, character)
38+
} else {
39+
if (INHERITS(lower, char_integer64))
40+
error(_("x is not integer64 but %s is. Please align classes."), "lower");
41+
if (INHERITS(upper, char_integer64))
42+
error(_("x is not integer64 but %s is. Please align classes."), "upper");
43+
}
44+
3245
if (isInteger(x)) {
3346
if ((isInteger(lower) || fitsInInt32(lower)) &&
3447
(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
90103

91104
case REALSXP:
92105
if (INHERITS(x, char_integer64)) {
93-
if (!INHERITS(lower, char_integer64) || !INHERITS(upper, char_integer64))
94-
error(_("x is integer64 but lower and/or upper are not.")); // e.g. between(int64, character, character)
95106
const int64_t *lp = (int64_t *)REAL(lower);
96107
const int64_t *up = (int64_t *)REAL(upper);
97108
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
117128
}
118129
if (verbose) Rprintf(_("between parallel processing of integer64 took %8.3fs\n"), omp_get_wtime()-tic);
119130
} else {
120-
if (INHERITS(lower, char_integer64) || INHERITS(upper, char_integer64))
121-
error(_("x is not integer64 but lower and/or upper is integer64. Please align classes."));
122131
const double *lp = REAL(lower);
123132
const double *up = REAL(upper);
124133
const double *xp = REAL(x);

‎src/utils.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ bool within_int64_repres(double x) {
1414
// used to error if not passed type double but this needed extra is.double() calls in calling R code
1515
// which needed a repeat of the argument. Hence simpler and more robust to return false when not type double.
1616
bool fitsInInt32(SEXP x) {
17-
if (!isReal(x))
17+
if (!isReal(x) || INHERITS(x, char_integer64))
1818
return false;
1919
R_xlen_t n=xlength(x), i=0;
2020
const double *dx = REAL(x);
@@ -31,7 +31,7 @@ SEXP fitsInInt32R(SEXP x) {
3131
}
3232

3333
bool fitsInInt64(SEXP x) {
34-
if (!isReal(x))
34+
if (!isReal(x) || INHERITS(x, char_integer64))
3535
return false;
3636
R_xlen_t n=xlength(x), i=0;
3737
const double *dx = REAL(x);

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL