Merge branch 'master' into todoRename

Author: badasahog

NEWS.md

@@ -87,7 +87,7 @@
 
 8. A data.table with a column of class `vctrs_list_of` (from package {vctrs}) prints as expected, [#5948](https://github.com/Rdatatable/data.table/issues/5948). Before, they could be printed messily, e.g. printing every entry in a nested data.frame. Thanks @jesse-smith for the report, @DavisVaughan and @r2evans for contributing, and @MichaelChirico for the PR.
 
-9.  Fixed incorrect sorting of merges where the first column of a key is a factor with non-`sort()`-ed levels (e.g. `factor(1:2, 2:1)` and it is joined to a character column, [#5361](https://github.com/Rdatatable/data.table/issues/5361). Thanks to @gbrunick for the report and Benjamin Schwendinger for the fix.
+9. Fixed incorrect sorting of merges where the first column of a key is a factor with non-`sort()`-ed levels (e.g. `factor(1:2, 2:1)` and it is joined to a character column, [#5361](https://github.com/Rdatatable/data.table/issues/5361). Thanks to @gbrunick for the report, Benjamin Schwendinger for the fix, and @MichaelChirico for a follow-up fix caught by revdep testing.
 
 10. Spurious warnings from internal code in `cube()`, `rollup()`, and `groupingsets()` are no longer surfaced to the caller, [#6964](https://github.com/Rdatatable/data.table/issues/6964). Thanks @ferenci-tamas for the report and @venom1204 for the fix.
  
@@ -122,6 +122,8 @@
 
 5. A GitHub Actions workflow is now in place to warn the entire maintainer team, as well as any contributor following the GitHub repository, when the package is at risk of archival on CRAN [#7008](https://github.com/Rdatatable/data.table/issues/7008). Thanks @tdhock for the original report and @Bisaloo and @TysonStanley for the fix.
 
+6. Using a double vector in `set()`'s `i=` and/or `j=` no longer throws a warning about preferring integer, [#6594](https://github.com/Rdatatable/data.table/issues/6594). While it may improve efficiency to use integer, there's no guarantee it's an improvement and the difference is likely to be minimal. The coercion will still be reported under `datatable.verbose=TRUE`. For package/production use cases, static analyzers such as `lintr::implicit_integer_linter()` can also report when numeric literals should be rewritten as integer literals.
+
 ## data.table [v1.17.8](https://github.com/Rdatatable/data.table/milestone/41) (6 July 2025)
 
 1. Internal functions used to signal errors are now marked as non-returning, silencing a compiler warning about potentially unchecked allocation failure. Thanks to Prof. Brian D. Ripley for the report and @aitap for the fix, [#7070](https://github.com/Rdatatable/data.table/pull/7070).

R/data.table.R

@@ -2047,8 +2047,9 @@ replace_dot_alias = function(e) {
   if (!.Call(CisOrderedSubset, irows, nrow(x)))
     return(NULL)
 
-  # see #1010. don't set key when i has no key, but irows is ordered and !roll
-  if (roll && length(irows) != 1L)
+  # see #1010. don't set key when i has no key, but irows is ordered and isFALSE(roll)
+  #   NB: roll could still be a string like 'nearest', #7146
+  if (!is.character(roll) && roll && length(irows) != 1L)
     return(NULL)
 
   new_key <- head(x_key, key_length)

inst/tests/tests.Rraw

@@ -1621,9 +1621,9 @@ test(514, x %chin% y, x %in% y)
 
 # Test new function set() in v1.8.0
 DT = data.table(a=1:3,b=4:6)
-test(515, set(DT,2,1,3), data.table(a=c(1L,3L,3L),b=4:6), warning=c("Coerced i from numeric to integer","Coerced j from numeric to integer"))
+test(515, set(DT,2,1,3), data.table(a=c(1L,3L,3L),b=4:6))
 test(516, set(DT,"2",1,3), error="i is type 'character'")
-test(517, set(DT,2L,1,3), DT, warning="Coerced j")
+test(517, options=c(datatable.verbose=TRUE), set(DT,2,1,3), DT, output="Coerced i.*Coerced j")
 # FR #2551 implemented - removed warning from 518
 # test(518, set(DT,2L,1L,3), DT, warning="Coerced 'double' RHS to 'integer'")
 test(518, set(DT,2L,1L,3), DT)
@@ -7131,6 +7131,11 @@ dt3 <- merge(dt1, dt2, by="x")
 test(1483.81, key(dt3), "x")
 test(1483.82, nrow(dt3[x %in% "c", ]), 2L)
 
+# avoid regression of roll='nearest' join of int->double, #7146
+DT1 <- data.table(a=1L, key='a')
+DT2 <- data.table(a=2.0, key='a')
+test(1483.83, DT1[DT2, roll='nearest'], data.table(a=2L, key='a'))
+
 # NULL items should be removed when making data.table from list, #842
 # Original fix for #842 added a branch in as.data.table.list() using point()
 # Then PR#3471 moved logic from data.table() into as.data.table.list() and now removes NULL items up front, so longer need for the branch

src/assign.c

@@ -219,7 +219,7 @@ SEXP setdt_nrows(SEXP x)
     if (Rf_inherits(xi, "POSIXlt")) {
       error(_("Column %d has class 'POSIXlt'. Please convert it to POSIXct (using as.POSIXct) and run setDT() again. We do not recommend the use of POSIXlt at all because it uses 40 bytes to store one date."), i+1);
     }
-    SEXP dim_xi = getAttrib(xi, R_DimSymbol);
+    SEXP dim_xi = PROTECT(getAttrib(xi, R_DimSymbol));
     R_len_t len_xi, n_dim = length(dim_xi);
     if (n_dim) {
       if (test_matrix_cols && n_dim > 1) {
@@ -230,6 +230,7 @@ SEXP setdt_nrows(SEXP x)
     } else {
       len_xi = length(xi);
     }
+    UNPROTECT(1);
     if (!base_length) {
       base_length = len_xi;
     } else if (len_xi != base_length) {
@@ -364,7 +365,8 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values)
   } else {
     if (isReal(rows)) {
       rows = PROTECT(coerceVector(rows, INTSXP)); protecti++;
-      warning(_("Coerced i from numeric to integer. Please pass integer for efficiency; e.g., 2L rather than 2"));
+      if (verbose)
+        Rprintf(_("Coerced %s from numeric to integer. Passing integer directly may be more efficient, e.g., 2L rather than 2"), "i");
     }
     if (!isInteger(rows))
       error(_("i is type '%s'. Must be integer, or numeric is coerced with warning. If i is a logical subset, simply wrap with which(), and take the which() outside the loop if possible for efficiency."), type2char(TYPEOF(rows)));
@@ -417,7 +419,8 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values)
   } else {
     if (isReal(cols)) {
       cols = PROTECT(coerceVector(cols, INTSXP)); protecti++;
-      warning(_("Coerced j from numeric to integer. Please pass integer for efficiency; e.g., 2L rather than 2"));
+      if (verbose)
+        Rprintf(_("Coerced %s from numeric to integer. Passing integer directly may be more efficient, e.g., 2L rather than 2"), "j");
     }
     if (!isInteger(cols))
       error(_("j is type '%s'. Must be integer, character, or numeric is coerced with warning."), type2char(TYPEOF(cols)));

src/inrange.c

@@ -1,16 +1,15 @@
 #include "data.table.h"
 #include <Rdefines.h>
 
-
-SEXP inrange(SEXP ansArg, SEXP xoArg, SEXP startsArg, SEXP lenArg) {
-
+SEXP inrange(SEXP ansArg, SEXP xoArg, SEXP startsArg, SEXP lenArg)
+{
   int *ans = INTEGER(ansArg);
   const int *xo = INTEGER(xoArg);
   const int *starts = INTEGER(startsArg), *len = INTEGER(lenArg);
   const int n = length(startsArg), nxo = length(xoArg);
-  for (int i=0; i<n; ++i) {
-    for (int j=starts[i]-1; j<starts[i]-1+len[i]; ++j) {
-      ans[nxo ? xo[j]-1 : j] = 1;
+  for (int i = 0; i < n; i++) {
+    for (int j = starts[i] - 1; j < starts[i] - 1 + len[i]; j++) {
+      ans[nxo ? xo[j] - 1 : j] = 1;
     }
   }
   // old complicated logic which is only really useful when matches
@@ -32,5 +31,5 @@ SEXP inrange(SEXP ansArg, SEXP xoArg, SEXP startsArg, SEXP lenArg) {
   //     // Rprintf(_("Moved to %d, start=%d, end=%d\n"), i, ss, ee);
   //     for (int j=ss; j<=ee; j++) ans[nxo ? xo[j]-1 : j] = 1;
   // }
-  return (R_NilValue);
+  return R_NilValue;
 }

src/negate.c

@@ -1,18 +1,19 @@
 #include "data.table.h"
 
-void negateByRef(SEXP x) {
+void negateByRef(SEXP x)
+{
   if(TYPEOF(x) != LGLSXP) {
     error(_("not logical or integer vector"));  // # nocov
   }
   const int n = length(x);
   int *ansd = LOGICAL(x);
-  for(int i=0; i<n; ++i) {
+  for(int i = 0; i < n; i++) {
     ansd[i] ^= (ansd[i] != NA_LOGICAL);  // invert true/false but leave NA alone
   }
 }
 
-
-SEXP notchin(SEXP x, SEXP table) {
+SEXP notchin(SEXP x, SEXP table)
+{
   // see discussion in PR#4931
   SEXP result = PROTECT(chin(x, table));
   negateByRef(result); // save memory

src/nqrecreateindices.c

@@ -2,9 +2,9 @@
 
 // todo: Add oxygen style comments and cleanup var names.
 // See other TODOs inside the function.
-SEXP nqRecreateIndices(SEXP xo, SEXP len, SEXP indices, SEXP nArg, SEXP nomatch) {
-
-  R_len_t n=INTEGER(nArg)[0], xn=length(xo);
+SEXP nqRecreateIndices(SEXP xo, SEXP len, SEXP indices, SEXP nArg, SEXP nomatch)
+{
+  R_len_t n = INTEGER(nArg)[0], xn = length(xo);
   SEXP ans, newstarts, newlen;
   ans = PROTECT(allocVector(VECSXP, 2));
   SET_VECTOR_ELT(ans, 0, (newstarts = allocVector(INTSXP, n)));
@@ -17,26 +17,28 @@ SEXP nqRecreateIndices(SEXP xo, SEXP len, SEXP indices, SEXP nArg, SEXP nomatch)
   const int inomatch = isNull(nomatch) ? 0 : INTEGER(nomatch)[0];
   int *inewstarts = INTEGER(newstarts);
 
-  for (int i=0; i<n; ++i) inewlen[i] = 0;
+  for (int i = 0; i < n; i++) inewlen[i] = 0;
+
   // simplifying logic ... also fixes #2275
-  for (int i=0; i<length(indices); ++i) {
-    inewlen[iindices[i]-1] += ilen[i];
+  for (int i = 0; i < length(indices); i++) {
+    inewlen[iindices[i] - 1] += ilen[i];
   }
   // fix for #2360, rewriting the for-loop from before
+
   // todo: revisit to see if this be simplified further when I've some time.
-  R_len_t j=0, tmp=0;
-  for (int i=0; i<n; ++i) {
-    if (j>=xn || ixo[j]<=0) {
+  R_len_t j = 0, tmp = 0;
+  for (int i = 0; i < n; i++) {
+    if (j >= xn || ixo[j] <= 0) {
       // NA_integer_ = INT_MIN is checked in init.c
       // j >= xn needed for special nomatch=NULL case, see issue#4388 (due to xo[irows] from R removing '0' value in xo)
       inewstarts[i] = inomatch;
       j++; // newlen will be 1 for xo=NA and 0 for xo=0 .. but we need to increment by 1 for both
     } else {
-      inewstarts[i] = tmp+1;
+      inewstarts[i] = tmp + 1;
       tmp += inewlen[i];
       j += inewlen[i];
     }
   }
   UNPROTECT(1);
-  return (ans);
+  return ans;
 }

src/openmp-utils.c

@@ -12,14 +12,14 @@ static bool RestoreAfterFork = true;  // see #2885 in v1.12.0
 static int getIntEnv(const char *name, int def)
 {
   const char *val = getenv(name);
-  if (val==NULL) return def;
+  if (val == NULL) return def;
   size_t nchar = strlen(val);
-  if (nchar==0) return def;
+  if (nchar == 0) return def;
   char *end;
   errno = 0;
   long int ans = strtol(val, &end, 10);  // ignores leading whitespace. If it fully consumed the string, *end=='\0' and isspace('\0')==false
   while (isspace(*end)) end++;  // ignore trailing whitespace
-  if (errno || (size_t)(end-val)!=nchar || ans<1 || ans>INT_MAX) {
+  if (errno || (size_t)(end - val) != nchar || ans < 1 || ans > INT_MAX) {
     warning(_("Ignoring invalid %s==\"%s\". Not an integer >= 1. Please remove any characters that are not a digit [0-9]. See ?data.table::setDTthreads."), name, val);
     return def;
   }
@@ -29,23 +29,24 @@ static int getIntEnv(const char *name, int def)
 static inline int imin(int a, int b) { return a < b ? a : b; }
 static inline int imax(int a, int b) { return a > b ? a : b; }
 
-void initDTthreads(void) {
+void initDTthreads(void)
+{
   // called at package startup from init.c
   // also called by setDTthreads(threads=NULL) (default) to reread environment variables; see setDTthreads below
   // No verbosity here in this setter. Verbosity is in getDTthreads(verbose=TRUE)
   int ans = getIntEnv("R_DATATABLE_NUM_THREADS", INT_MIN);
-  if (ans>=1) {
+  if (ans >= 1) {
     ans = imin(ans, omp_get_num_procs());  // num_procs is a hard limit; user cannot achieve more. ifndef _OPENMP then myomp.h defines this to be 1
   } else {
     // Only when R_DATATABLE_NUM_THREADS is unset (or <=0) do we use PROCS_PERCENT; #4514
     int perc = getIntEnv("R_DATATABLE_NUM_PROCS_PERCENT", 50); // use "NUM_PROCS" to use the same name as the OpenMP function this uses
     // 50% of logical CPUs by default; half of 8 is 4 on laptop with 4 cores. Leaves plenty of room for other processes: #3395 & #3298
-    if (perc<=1 || perc>100) {
+    if (perc <= 1 || perc > 100) {
       warning(_("Ignoring invalid R_DATATABLE_NUM_PROCS_PERCENT==%d. If used it must be an integer between 2 and 100. Default is 50. See ?setDTtheads."), perc);
       // not allowing 1 is to catch attempts to use 1 or 1.0 to represent 100%.
       perc = 50;
     }
-    ans = imax(omp_get_num_procs()*perc/100, 1); // imax for when formula would result in 0.
+    ans = imax(omp_get_num_procs() * perc / 100, 1); // imax for when formula would result in 0.
   }
   ans = imin(ans, omp_get_thread_limit());  // honors OMP_THREAD_LIMIT when OpenMP started; e.g. CRAN sets this to 2. Often INT_MAX meaning unlimited/unset
   ans = imin(ans, omp_get_max_threads());   // honors OMP_NUM_THREADS when OpenMP started, plus reflects any omp_set_* calls made since
@@ -57,24 +58,27 @@ void initDTthreads(void) {
   DTthrottle = imax(1, getIntEnv("R_DATATABLE_THROTTLE", 1024)); // 2nd thread is used only when n>1024, 3rd thread when n>2048, etc
 }
 
-int getDTthreads(const int64_t n, const bool throttle) {
+int getDTthreads(const int64_t n, const bool throttle)
+{
   // this is the main getter used by all parallel regions; they specify num_threads(n, true|false).
   // Keep this light, simple and robust. initDTthreads() ensures 1 <= DTthreads <= omp_get_num_proc()
   // throttle introduced in 1.12.10 (see NEWS item); #4484
   // throttle==true  : a number of iterations per thread (DTthrottle) is applied before a second thread is utilized
   // throttle==false : parallel region is already pre-chunked such as in fread; e.g. two batches intended for two threads
-  if (n<1) return 1; // 0 or negative could be deliberate in calling code for edge cases where loop is not intended to run at all
-  int64_t ans = throttle ? 1+(n-1)/DTthrottle :  // 1 thread for n<=1024, 2 thread for n<=2048, etc
+  if (n < 1) return 1; // 0 or negative could be deliberate in calling code for edge cases where loop is not intended to run at all
+  int64_t ans = throttle ? 1 + (n - 1) / DTthrottle :  // 1 thread for n<=1024, 2 thread for n<=2048, etc
                            n;                    // don't use 20 threads for just one or two batches
-  return ans>=DTthreads ? DTthreads : (int)ans;  // apply limit in static local DTthreads saved there by initDTthreads() and setDTthreads()
+  return ans >= DTthreads ? DTthreads : (int)ans;  // apply limit in static local DTthreads saved there by initDTthreads() and setDTthreads()
 }
 
-static const char *mygetenv(const char *name, const char *unset) {
+static const char *mygetenv(const char *name, const char *unset)
+{
   const char *ans = getenv(name);
-  return (ans==NULL || ans[0]=='\0') ? unset : ans;
+  return (ans == NULL || ans[0] == '\0') ? unset : ans;
 }
 
-SEXP getDTthreads_R(SEXP verbose) {
+SEXP getDTthreads_R(SEXP verbose)
+{
   if(!IS_TRUE_OR_FALSE(verbose))
     error(_("%s must be TRUE or FALSE"), "verbose");
   if (LOGICAL(verbose)[0]) {
@@ -102,15 +106,16 @@ SEXP getDTthreads_R(SEXP verbose) {
   return ScalarInteger(getDTthreads(INT_MAX, false));
 }
 
-SEXP setDTthreads(SEXP threads, SEXP restore_after_fork, SEXP percent, SEXP throttle) {
+SEXP setDTthreads(SEXP threads, SEXP restore_after_fork, SEXP percent, SEXP throttle)
+{
   if (!isNull(restore_after_fork)) {
-    if (!isLogical(restore_after_fork) || LOGICAL(restore_after_fork)[0]==NA_LOGICAL) {
+    if (!isLogical(restore_after_fork) || LOGICAL(restore_after_fork)[0] == NA_LOGICAL) {
       error(_("restore_after_fork= must be TRUE, FALSE, or NULL (default). getDTthreads(verbose=TRUE) reports the current setting.\n"));
     }
     RestoreAfterFork = LOGICAL(restore_after_fork)[0];  // # nocov
   }
   if (length(throttle)) {
-    if (!isInteger(throttle) || LENGTH(throttle)!=1 || INTEGER(throttle)[0]<1)
+    if (!isInteger(throttle) || LENGTH(throttle) != 1 || INTEGER(throttle)[0] < 1)
       error(_("'throttle' must be a single number, non-NA, and >=1"));
     DTthrottle = INTEGER(throttle)[0];
   }
@@ -123,19 +128,19 @@ SEXP setDTthreads(SEXP threads, SEXP restore_after_fork, SEXP percent, SEXP thro
     // If a CPU has been unplugged (high end servers allow live hardware replacement) then omp_get_num_procs() will
     // reflect that and a call to setDTthreads(threads=NULL) will update DTthreads.
   } else if (length(threads)) {
-    int n=0;
-    if (length(threads)!=1 || !isInteger(threads) || (n=INTEGER(threads)[0]) < 0) {  // <0 catches NA too since NA is negative (INT_MIN)
+    int n = 0;
+    if (length(threads) != 1 || !isInteger(threads) || (n = INTEGER(threads)[0]) < 0) {  // <0 catches NA too since NA is negative (INT_MIN)
       error(_("threads= must be either NULL or a single number >= 0. See ?setDTthreads."));
     }
     int num_procs = imax(omp_get_num_procs(), 1); // max just in case omp_get_num_procs() returns <= 0 (perhaps error, or unsupported)
-    if (!isLogical(percent) || length(percent)!=1 || LOGICAL(percent)[0]==NA_LOGICAL) {
+    if (!isLogical(percent) || length(percent) != 1 || LOGICAL(percent)[0] == NA_LOGICAL) {
       internal_error(__func__, "percent= must be TRUE or FALSE at C level");  // # nocov
     }
     if (LOGICAL(percent)[0]) {
-      if (n<2 || n>100) internal_error(__func__, "threads==%d should be between 2 and 100 (percent=TRUE at C level)", n);  // # nocov
-      n = num_procs*n/100;  // if 0 it will be reset to 1 in the imax() below
+      if (n < 2 || n > 100) internal_error(__func__, "threads==%d should be between 2 and 100 (percent=TRUE at C level)", n);  // # nocov
+      n = num_procs * n / 100;  // if 0 it will be reset to 1 in the imax() below
     } else {
-      if (n==0 || n>num_procs) n = num_procs; // setDTthreads(0) == setDTthread(percent=100); i.e. use all logical CPUs (the default in 1.12.0 and before, from 1.12.2 it's 50%)
+      if (n == 0 || n > num_procs) n = num_procs; // setDTthreads(0) == setDTthread(percent=100); i.e. use all logical CPUs (the default in 1.12.0 and before, from 1.12.2 it's 50%)
     }
     n = imin(n, omp_get_thread_limit());  // can't think why this might be different from its value on startup, but call it just in case
     n = imin(n, getIntEnv("OMP_THREAD_LIMIT", INT_MAX));  // user might have called Sys.setenv(OMP_THREAD_LIMIT=) since startup and expect setDTthreads to respect it
@@ -171,16 +176,19 @@ SEXP setDTthreads(SEXP threads, SEXP restore_after_fork, SEXP percent, SEXP thro
 
 static int pre_fork_DTthreads = 0;
 
-void when_fork(void) {
+void when_fork(void)
+{
   pre_fork_DTthreads = DTthreads;
   DTthreads = 1;
 }
 
-void after_fork(void) {
+void after_fork(void)
+{
   if (RestoreAfterFork) DTthreads = pre_fork_DTthreads;
 }
 
-void avoid_openmp_hang_within_fork(void) {
+void avoid_openmp_hang_within_fork(void)
+{
   // Called once on loading data.table from init.c
 #ifdef _OPENMP
   pthread_atfork(&when_fork, &after_fork, NULL);

src/programming.c

@@ -1,6 +1,7 @@
 #include "data.table.h"
 
-static void substitute_call_arg_names(SEXP expr, SEXP env) {
+static void substitute_call_arg_names(SEXP expr, SEXP env)
+{
   if (!length(expr) || !isLanguage(expr))
     return; // isLanguage is R's is.call
   SEXP arg_names = getAttrib(expr, R_NamesSymbol);
@@ -9,17 +10,17 @@ static void substitute_call_arg_names(SEXP expr, SEXP env) {
     int *imatches = INTEGER(PROTECT(chmatch(arg_names, env_names, 0)));
     const SEXP *env_sub = SEXPPTR_RO(env);
     SEXP tmp = expr;
-    for (int i=0; i<length(arg_names); i++, tmp=CDR(tmp)) { // substitute call arg names
+    for (int i = 0; i < length(arg_names); i++, tmp = CDR(tmp)) { // substitute call arg names
       if (!imatches[i])
         continue;
-      SEXP sym = env_sub[imatches[i]-1];
+      SEXP sym = env_sub[imatches[i] - 1];
       if (!isSymbol(sym))
         error(_("Attempting to substitute '%s' element with object of type '%s' but it has to be 'symbol' type when substituting name of the call argument, functions 'as.name' and 'I' can be used to work out proper substitution, see ?substitute2 examples."), CHAR(STRING_ELT(arg_names, i)), type2char(TYPEOF(sym)));
       SET_TAG(tmp, sym);
     }
     UNPROTECT(1); // chmatch
   }
-  for (SEXP tmp=expr; tmp!=R_NilValue; tmp=CDR(tmp)) { // recursive call to substitute in nested expressions
+  for (SEXP tmp = expr; tmp != R_NilValue; tmp = CDR(tmp)) { // recursive call to substitute in nested expressions
     substitute_call_arg_names(CADR(tmp), env);
   }
 }