Merge branch 'Rdatatable:master' into issue5829

Author: venom1204

.ci/.lintr.R

@@ -16,6 +16,13 @@ linters = c(dt_linters, all_linters(
   # system_time_linter = undesirable_function_linter(c(
   #   system.time = "Only run timings in benchmark.Rraw"
   # )),
+  undesirable_function_linter(c(
+    cat = "Use catf to enable translations",
+    message = "Use messagef to avoid fragmented translations.",
+    warning = "Use warningf to avoid fragmented translations.",
+    stop = "Use stopf to avoid fragmented translations.",
+    NULL
+  )),
   # undesirable_function_linter(modify_defaults(
   #   default_undesirable_functions,
   #   ifelse = "Use fifelse instead.",

.ci/README.md

@@ -12,11 +12,15 @@ Test jobs:
 - `test-lin-rel-cran` - `--as-cran` on Linux, strict test for final status of `R CMD check`.
 - `test-lin-dev-gcc-strict-cran` - `--as-cran` on Linux, `r-devel` built with `-enable-strict-barrier --disable-long-double`, test for compilation warnings, test for new NOTEs/WARNINGs from `R CMD check`.
 - `test-lin-dev-clang-cran` - same as `gcc-strict` job but R built with `clang` and  no `--enable-strict-barrier --disable-long-double` flags.
-- `test-lin-310-cran` - R 3.1.0 on Linux, stated R dependency version.
+- `test-lin-ancient-cran` - Stated R dependency version (currently 3.4.0) on Linux.
+- `test-lin-dev-san` - `r-devel` on Linux built with `clang -fsanitize=address,undefined` (including LeakSanitizer), test for sanitizer output in tests and examples.
 - `test-win-rel` - `r-release` on Windows.
 - `test-win-dev` - `r-devel` on Windows.
 - `test-win-old` - `r-oldrel` on Windows.
-- `test-mac-rel` - macOS build not yet available, see [#3326](https://github.com/Rdatatable/data.table/issues/3326) for status
+- `test-mac-rel` - `r-release` on macOS.
+- `test-mac-old` - `r-oldrel` on macOS.
+
+The CI steps for the tests are [required](https://github.com/Rdatatable/data.table/blob/55eb0f160b169398d51f138131c14a66c86e5dc9/.ci/publish.R#L162-L168) to be named according to the pattern `test-(lin|win|mac)-<R version>[-<suffix>]*`, where `<R version>` is `rel`, `dev`, `old`, `ancient`, or three digits comprising an R version (e.g. `362` corresponding to R-3.6.2).
 
 Tests jobs are allowed to fail, summary and logs of test jobs are later published at _CRAN-like checks_ page, see artifacts below.
 

.ci/atime/tests.R

@@ -1,3 +1,5 @@
+pval.thresh <- 0.001 # to reduce false positives.
+
 # Test case adapted from https://github.com/Rdatatable/data.table/issues/6105#issue-2268691745 which is where the issue was reported.
 # https://github.com/Rdatatable/data.table/pull/6107 fixed performance across 3 ways to specify a column as Date, and we test each individually.
 extra.args.6107 <- c(
@@ -13,6 +15,7 @@ for (extra.arg in extra.args.6107){
       tmp_csv = tempfile()
       fwrite(DT, tmp_csv)
     },
+    FasterIO = "60a01fa65191c44d7997de1843e9a1dfe5be9f72", # First commit of the PR (https://github.com/Rdatatable/data.table/pull/6925/commits) that reduced time usage
     Slow = "e9087ce9860bac77c51467b19e92cf4b72ca78c7", # Parent of the merge commit (https://github.com/Rdatatable/data.table/commit/a77e8c22e44e904835d7b34b047df2eff069d1f2) of the PR (https://github.com/Rdatatable/data.table/pull/6107) that fixes the issue
     Fast = "a77e8c22e44e904835d7b34b047df2eff069d1f2") # Merge commit of the PR (https://github.com/Rdatatable/data.table/pull/6107) that fixes the issue
   this.test$expr = str2lang(sprintf("data.table::fread(tmp_csv, %s)", extra.arg))
@@ -117,12 +120,29 @@ test.list <- atime::atime_test_list(
       file.path("src", "init.c"),
       paste0("R_init_", Package_regex),
       paste0("R_init_", gsub("[.]", "_", new.Package_)))
+    # allow compilation on new R versions where 'Calloc' is not defined
+    pkg_find_replace(
+      file.path("src", "*.c"),
+      "\\b(Calloc|Free|Realloc)\\b",
+      "R_\\1")
     pkg_find_replace(
       "NAMESPACE",
       sprintf('useDynLib\\("?%s"?', Package_regex),
       paste0('useDynLib(', new.Package_))
   },
 
+  # Constant overhead improvement https://github.com/Rdatatable/data.table/pull/6925
+  # Test case adapted from https://github.com/Rdatatable/data.table/pull/7022#discussion_r2107900643
+  "fread disk overhead improved in #6925" = atime::atime_test(
+    N = 2^seq(0, 20), # smaller N because we are doing multiple fread calls.
+    setup = {
+      fwrite(iris[1], iris.csv <- tempfile())
+    },
+    expr = replicate(N, data.table::fread(iris.csv)),
+    Fast = "60a01fa65191c44d7997de1843e9a1dfe5be9f72", # First commit of the PR (https://github.com/Rdatatable/data.table/pull/6925/commits) that reduced time usage
+    Slow = "e25ea80b793165094cea87d946d2bab5628f70a6" # Parent of the first commit (https://github.com/Rdatatable/data.table/commit/60a01fa65191c44d7997de1843e9a1dfe5be9f72)
+  ),
+
   # Performance regression discussed in https://github.com/Rdatatable/data.table/issues/4311
   # Test case adapted from https://github.com/Rdatatable/data.table/pull/4440#issuecomment-632842980 which is the fix PR.
   "shallow regression fixed in #4440" = atime::atime_test(
@@ -172,8 +192,9 @@ test.list <- atime::atime_test_list(
   # Fixed in https://github.com/Rdatatable/data.table/pull/4558
   "DT[by] fixed in #4558" = atime::atime_test(
     setup = {
+      N9 <- as.integer(N * 0.9)
       d <- data.table(
-        id = sample(c(seq.int(N * 0.9), sample(N * 0.9, N * 0.1, TRUE))),
+        id = sample(c(seq.int(N9), sample(N9, N-N9, TRUE))),
         v1 = sample(5L, N, TRUE),
         v2 = sample(5L, N, TRUE)
       )
@@ -246,5 +267,15 @@ test.list <- atime::atime_test_list(
     Before = "f339aa64c426a9cd7cf2fcb13d91fc4ed353cd31", # Parent of the first commit https://github.com/Rdatatable/data.table/commit/fcc10d73a20837d0f1ad3278ee9168473afa5ff1 in the PR https://github.com/Rdatatable/data.table/pull/6393/commits with major change to fwrite with gzip.
     PR = "3630413ae493a5a61b06c50e80d166924d2ef89a"), # Close-to-last merge commit in the PR.
 
-  tests=extra.test.list)
+  # Test case created directly using the atime code below (not adapted from any other benchmark), based on the PR, Removes unnecessary data.table call from as.data.table.array https://github.com/Rdatatable/data.table/pull/7010 
+  "as.data.table.array improved in #7010" = atime::atime_test(
+    setup = {
+      dims = c(N, 1, 1)
+      arr = array(seq_len(prod(dims)), dim=dims)
+    },
+    expr = data.table:::as.data.table.array(arr, na.rm=FALSE),
+    Slow = "73d79edf8ff8c55163e90631072192301056e336",   # Parent of the first commit in the PR (https://github.com/Rdatatable/data.table/commit/8397dc3c993b61a07a81c786ca68c22bc589befc)
+    Fast = "8397dc3c993b61a07a81c786ca68c22bc589befc"),  # Commit in the PR (https://github.com/Rdatatable/data.table/pull/7019/commits) that removes inefficiency
+
+    tests=extra.test.list)
 # nolint end: undesirable_operator_linter.

.ci/linters/c/alloc_linter.R

@@ -6,7 +6,7 @@ alloc_linter = function(c_obj) {
   lines = c_obj$lines
   # Be a bit more precise to avoid mentions in comments, and allow
   #   malloc(0) to be used for convenience (e.g. #6757)
-  alloc_lines = grep(R"{=\s*([(]\w+\s*[*][)])?[mc]alloc[(][^0]}", lines)
+  alloc_lines = grep(R"{=\s*([(]\s*\w+\s*[*]\s*[)])?\s*[mc]alloc[(][^0]}", lines)
   if (!length(alloc_lines)) return()
   # int *tmp=(int*)malloc(...); or just int tmp=malloc(...);
   alloc_keys = lines[alloc_lines] |>
@@ -31,7 +31,7 @@ alloc_linter = function(c_obj) {
       cat("FILE: ", c_obj$path, "; LINES: ", head(bad_lines_idx, 1L), "-", tail(bad_lines_idx, 1L), "\n", sep="")
       writeLines(lines[bad_lines_idx])
       cat(strrep("-", max(nchar(lines[bad_lines_idx]))), "\n", sep="")
-      stop("Expected the malloc()/calloc() usage above to be followed immediately by error checking.", call.=FALSE)
+      stop("Expected the malloc()/calloc() usage above to be followed immediately by error checking (using '!', not '==NULL').", call.=FALSE)
     }
   })
 }

.ci/linters/c/cocci_linter.R

@@ -1,20 +1,23 @@
 cocci_linter = if (!nzchar(Sys.which("spatch"))) function(...) {} else function(c_obj) {
-  bad <- FALSE
+  bad = FALSE
+  tmp = tempfile(fileext = '.c')
+  on.exit(unlink(tmp))
+  writeLines(c_obj$preprocessed, tmp)
   for (spfile in list.files(".ci/linters/cocci", full.names = TRUE)) {
-    # Coccinelle parser gets confused sometimes, so ignore stderr and the exit code
-    out = suppressWarnings(system2(
+    out = system2(
       "spatch",
-      shQuote(c(
-        "--sp-file", spfile, c_obj$path, "--recursive-includes",
-        "-I", R.home("include"), "-I", "src"
-      )),
+      shQuote(c("--sp-file", spfile, tmp)),
       stdout = TRUE, stderr = FALSE
-    ))
+    )
     if (length(out) > 0) {
-      cat(sprintf("In file '%s', Coccinelle patch '%s' recommends the following changes:\n", c_obj$path, spfile))
+      cat(sprintf("In file '%s', Coccinelle linter '%s' located the following problems:\n", c_obj$path, spfile))
       writeLines(out)
-      bad <- TRUE
+      bad = TRUE
+    }
+    if (!is.null(status <- attr(out, 'status'))) {
+      cat(sprintf("While working on file '%s', Coccinelle linter '%s' failed with exit code %d:\n", c_obj$path, spfile, status))
+      bad = TRUE
     }
   }
-  if (bad) stop("Please apply the changes above or fix the linter")
+  if (bad) stop("Please investigate the problems above.")
 }

.ci/linters/cocci/malloc_return_value_cast.cocci

@@ -0,0 +1,14 @@
+@malloc_return_value_cast expression@
+type T;
+expression E;
+@@
+- (T)
+  malloc(E)
+
+@calloc_realloc_return_value_cast expression@
+type T;
+expression E1, E2;
+identifier alloc =~ "^(c|re)alloc$";
+@@
+- (T)
+  alloc(E1, E2)

.dev/CRAN_Release.cmd

@@ -95,17 +95,6 @@ grep "PROTECT_PTR" ./src/*.c
 # No use of long long, instead use int64_t. TODO
 # grep "long long" ./src/*.c
 
-// No use of llu, lld, zd or zu
-grep -nE "(llu|lld|zd|zu)" src/*.[hc]
-// Comment moved here from fread.c on 19 Nov 2019
-// [Moved from fread.c on 19 Nov 2019] On Windows variables of type `size_t` cannot be printed
-// with "%zu" in the `snprintf()` function. For those variables we used to cast them into
-// `unsigned long long int` before printing, and defined (llu) to make the cast shorter.
-// We're now observing warnings from gcc-8 with -Wformat-extra-args, #4062. So
-// now we're more strict and cast to [u]int64_t and use PRIu64/PRId64 from <inttypes.h>
-// In many cases the format specifier is passed to our own macro (e.g. DTPRINT) or to Rprintf(),
-// error() etc, and even if they don't call sprintf() now, they could in future.
-
 # No tabs in C or R code (sorry, Richard Hendricks)
 grep -P "\t" ./R/*.R
 grep -P "\t" ./src/*.c
@@ -235,23 +224,23 @@ system.time(test.data.table(script="*.Rraw"))  # apx 8h = froll 3h + nafill 1m +
 
 
 ###############################################
-#  R 3.3.0 (stated dependency)
+#  R 3.4.0 (stated dependency)
 ###############################################
 
 ### ONE TIME BUILD
 sudo apt-get -y build-dep r-base
 cd ~/build
-wget http://cran.stat.ucla.edu/src/base/R-3/R-3.3.0.tar.gz
-tar xvf R-3.3.0.tar.gz
-cd R-3.3.0
+wget http://cran.stat.ucla.edu/src/base/R-3/R-3.4.0.tar.gz
+tar xvf R-3.4.0.tar.gz
+cd R-3.4.0
 CFLAGS="-fcommon" FFLAGS="-fallow-argument-mismatch" ./configure --without-recommended-packages
 make
-alias R330=~/build/R-3.3.0/bin/R
+alias R340=~/build/R-3.4.0/bin/R
 ### END ONE TIME BUILD
 
 cd ~/GitHub/data.table
-R330 CMD INSTALL ./data.table_1.16.99.tar.gz
-R330
+R340 CMD INSTALL ./data.table_1.16.99.tar.gz
+R340
 require(data.table)
 test.data.table(script="*.Rraw")
 

.dev/README.md

@@ -1,5 +1,25 @@
 # data.table developer
 
+Inside this repository we provide some tools to improves development experience. Most notable is the `cc()` helper function that recompiles C sources, reloads R sources, and runs tests.
+
+Typical development workflow will then look like:
+
+0. `git checkout -b [branch]`
+1. edit package files
+2. run `R`
+3. call `cc(TRUE)`
+4. (if needed) go to point 1.
+
+Once we (and tests) are satisfied with changes, we then run complete package checks:
+
+0. in shell terminal
+1. run `make build`
+2. run `make check`
+3. (optionally) run on `r-devel`, e.g. `R=~/build/R-devel/bin/R make check`
+4. (optionally) run on ancient R, e.g. `R=~/build/R-340/bin/R make check`
+5. `git commit -m '[changes description]'`
+6. `git push [remote] [branch]`
+
 ## Setup
 
 To use the optional helper function `cc()`, one needs to set up the project path and source `.dev/cc.R` to use `cc()` conveniently. This works through creating an additional `.Rprofile` in the `data.table` directory.

.dev/lsan.supp

@@ -0,0 +1 @@
+leak:libfontconfig.so

.devcontainer/r-ancient-gcc/Dockerfile

@@ -1,4 +1,4 @@
-FROM registry.gitlab.com/jangorecki/dockerfiles/r-3.3.0
+FROM registry.gitlab.com/jangorecki/dockerfiles/r-3.4.0
 
 RUN apt-get -qq update \
   && apt-get install -y --no-install-recommends git