Merge branch 'master' into mergepair

Author: MichaelChirico

.Rbuildignore

@@ -1,4 +1,5 @@
 .dir-locals.el
+.check.translations.R
 ^\.Rprofile$
 ^data\.table_.*\.tar\.gz$
 ^config\.log$

.ci/.lintr.R

@@ -9,13 +9,20 @@ linters = c(dt_linters, all_linters(
   packages = "lintr", # TODO(lintr->3.2.0): Remove this.
   # eq_assignment_linter(),
   brace_linter(allow_single_line = TRUE),
+  implicit_integer_linter(allow_colon = TRUE),
   # TODO(michaelchirico): Activate these incrementally. These are the
   #   parameterizations that match our style guide.
   # implicit_assignment_linter(allow_lazy = TRUE, allow_scoped = TRUE),
-  # implicit_integer_linter(allow_colon = TRUE),
   # 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.",
@@ -26,7 +33,8 @@ linters = c(dt_linters, all_linters(
   #   setwd = NULL
   # )),
   undesirable_operator_linter(),
-  # TODO(lintr#2441): Use upstream implementation.
+  # TODO(lintr#2765): Use upstream implementation.
+  # assignment_linter(operator = "="),
   assignment_linter = NULL,
   absolute_path_linter = NULL, # too many false positives
   # TODO(lintr#2442): Use this once x[ , j, by] is supported.
@@ -70,43 +78,39 @@ linters = c(dt_linters, all_linters(
 ))
 rm(dt_linters)
 
-# TODO(lintr#2172): Glob with lintr itself.
-exclusions = c(local({
-  exclusion_for_dir <- function(dir, exclusions) {
-    files = file.path("..", list.files(dir, pattern = "\\.(R|Rmd|Rraw)$", full.names=TRUE))
-    stats::setNames(rep(list(exclusions), length(files)), files)
-  }
-  c(
-    exclusion_for_dir("tests", list(
-      quotes_linter = Inf,
-      # TODO(michaelchirico): Enforce these and re-activate them one-by-one.
-      implicit_integer_linter = Inf,
-      infix_spaces_linter = Inf,
-      undesirable_function_linter = Inf
-    )),
-    exclusion_for_dir("vignettes", list(
-      quotes_linter = Inf,
-      sample_int_linter = Inf
-      # strings_as_factors_linter = Inf
-      # system_time_linter = Inf
-    )),
-    exclusion_for_dir("inst/tests", list(
-      library_call_linter = Inf,
-      numeric_leading_zero_linter = Inf,
-      undesirable_operator_linter = Inf, # For ':::', possibly we could be more careful to only exclude ':::'.
-      # TODO(michaelchirico): Enforce these and re-activate them one-by-one.
-      comparison_negation_linter = Inf,
-      condition_call_linter = Inf,
-      duplicate_argument_linter = Inf,
-      equals_na_linter = Inf,
-      missing_argument_linter = Inf,
-      paste_linter = Inf,
-      rep_len_linter = Inf,
-      sample_int_linter = Inf,
-      seq_linter = Inf,
-      unnecessary_lambda_linter = Inf
-    ))
+exclusions = list(
+  `../tests` = list(
+    quotes_linter = Inf,
+    # TODO(michaelchirico): Enforce these and re-activate them one-by-one.
+    implicit_integer_linter = Inf,
+    infix_spaces_linter = Inf,
+    undesirable_function_linter = Inf
+  ),
+  `../vignettes*` = list(
+    # assignment_linter = Inf,
+    implicit_integer_linter = Inf,
+    quotes_linter = Inf,
+    sample_int_linter = Inf
+    # strings_as_factors_linter = Inf
+    # system_time_linter = Inf
+  ),
+  `../inst/tests` = list(
+    library_call_linter = Inf,
+    numeric_leading_zero_linter = Inf,
+    undesirable_operator_linter = Inf, # For ':::', possibly we could be more careful to only exclude ':::'.
+    # TODO(michaelchirico): Enforce these and re-activate them one-by-one.
+    comparison_negation_linter = Inf,
+    condition_call_linter = Inf,
+    duplicate_argument_linter = Inf,
+    equals_na_linter = Inf,
+    missing_argument_linter = Inf,
+    paste_linter = Inf,
+    rep_len_linter = Inf,
+    sample_int_linter = Inf,
+    seq_linter = Inf,
+    unnecessary_lambda_linter = Inf
+  ),
+  `../inst/tests/froll.Rraw` = list(
+    dt_test_literal_linter = Inf # TODO(michaelchirico): Fix these once #5898, #5692, #5682, #5576, #5575, #5441 are merged.
   )
-}),
-  list(`../inst/tests/froll.Rraw` = list(dt_test_literal_linter = Inf)) # TODO(michaelchirico): Fix these once #5898, #5692, #5682, #5576, #5575, #5441 are merged.
 )

.ci/README.md

@@ -1,6 +1,6 @@
 # data.table continuous integration and deployment
 
-On each Pull Request opened in GitHub we run GitHub Actions test jobs to provide prompt feedback about the status of PR. Our main CI pipeline runs on GitLab CI nightly. GitLab repository automatically mirrors our GitHub repository and runs pipeline on `master` branch every night. It tests more environments and different configurations. It publish variety of artifacts.
+On each Pull Request opened in GitHub we run GitHub Actions test jobs to provide prompt feedback about the status of PR. Our more thorough main CI pipeline runs nightly on GitLab CI. In addition to branches pushed directly, the GitLab repository automatically mirrors our GitHub repository and runs pipeline on the `master` branch every night. It tests more environments and different configurations. It publishes a variety of artifacts such as our [homepage](https://rdatatable.gitlab.io/data.table/) and [CRAN-like website for dev version](https://rdatatable.gitlab.io/data.table/web/packages/data.table/index.html), including windows binaries for the dev version.
 
 ## Environments
 
@@ -12,11 +12,16 @@ 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-clang-san` - `r-devel` on Linux built with `clang -fsanitize=address,undefined` (including LeakSanitizer), test for sanitizer output in tests and examples.
+- `test-lin-dev-gcc-san` - `r-devel` on Linux built with `gcc -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.
 
@@ -44,3 +49,39 @@ Base R implemented helper script, [originally proposed to base R](https://svn.r-
 ### [`publish.R`](./publish.R)
 
 Base R implemented helper script to orchestrate generation of most artifacts and to arrange them nicely. It is being used only in [_integration_ stage in GitLab CI pipeline](./../.gitlab-ci.yml).
+
+### [`lint.R`](./lint.R)
+
+Base R runner for the manual (non-`lintr`) lint checks to be run from GitHub Actions during the code quality check. The command line arguments are as follows:
+1. Path to the directory containing files defining the linters. A linter is a function that accepts one argument (typically the path to the file) and signals an error if it fails the lint check.
+2. Path to the directory containing files to check.
+3. A regular expression matching the files to check.
+
+One of the files in the linter directory may define the `.preprocess` function, which must accept one file path and return a value that other linter functions will understand. The function may also return `NULL` to indicate that the file must be skipped.
+
+Example command lines:
+
+```sh
+Rscript .ci/lint.R .ci/linters/c src '[.][ch]$'
+Rscript .ci/lint.R .ci/linters/po po '[.]po$'
+Rscript .ci/lint.R .ci/linters/md . '[.]R?md$'
+```
+
+## GitLab Open Source Program
+
+We are currently part of the [GitLab for Open Source Program](https://about.gitlab.com/solutions/open-source/). This gives us 50,000 compute minutes per month for our GitLab CI. Our license needs to be renewed yearly (around July) and is currently managed by @ben-schwen.
+
+## Updating CI pipeline
+
+Basic CI checks are also run on every push to the GitLab repository. This can **and should** be used for PRs changing the CI pipeline before merging them to master.
+
+```shell
+# fetch changes from remote (GitHub) and push them to GitLab 
+git fetch [email protected]:Rdatatable/data.table.git new_branch:new_branch
+git push
+# after updating on GitHub, pull changes from remote and push to GitLab
+git pull [email protected]:Rdatatable/data.table.git new_branch
+git push
+```
+
+Make sure to include a link to the pipeline results in your PR. 

.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,12 +15,38 @@ 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))
   extra.test.list[[sprintf("fread(%s) improved in #6107", extra.arg)]] <- this.test
 }
 
+# Test case adapted from https://github.com/Rdatatable/data.table/pull/4386#issue-602528139 which is where the performance was improved.
+for(retGrp_chr in c("T","F"))extra.test.list[[sprintf(
+  "forderv(retGrp=%s) improved in #4386", retGrp_chr
+)]] <- list(
+  setup = quote({
+    dt <- data.table(group = rep(1:2, l=N))
+  }),
+  expr = substitute({
+    old.opt <- options(datatable.forder.auto.index = TRUE) # required for test, un-documented, comments in forder.c say it is for debugging only.
+    data.table:::forderv(dt, "group", retGrp = RETGRP)
+    options(old.opt) # so the option does not affect other tests.
+  }, list(RETGRP=eval(str2lang(retGrp_chr)))),
+  ## From ?bench::mark, "Each expression will always run at least twice,
+  ## once to measure the memory allocation and store results
+  ## and one or more times to measure timing."
+  ## So for atime(times=10) that means 11 times total.
+  ## First time for memory allocation measurement,
+  ## (also sets the index of dt in this example),
+  ## then 10 more times for time measurement.
+  ## Timings should be constant if the cached index is used (Fast),
+  ## and (log-)linear if the index is re-computed (Slow).
+  Slow = "b1b1832b0d2d4032b46477d9fe6efb15006664f4", # Parent of the first commit (https://github.com/Rdatatable/data.table/commit/b0efcf59442a7d086c6df17fa6a45c81b082322e) in the PR (https://github.com/Rdatatable/data.table/pull/4386/commits) where the performance was improved.
+  Fast = "ffe431fbc1fe2d52ed9499f78e7e16eae4d71a93" # Last commit of the PR (https://github.com/Rdatatable/data.table/pull/4386/commits) where the performance was improved.
+)
+
 # A list of performance tests.
 #
 # See documentation in https://github.com/Rdatatable/data.table/wiki/Performance-testing for best practices.
@@ -92,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(
@@ -147,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)
       )
@@ -206,7 +252,30 @@ test.list <- atime::atime_test_list(
     },
     expr = data.table:::melt(DT, measure.vars = measure.vars),
     Slow = "fd24a3105953f7785ea7414678ed8e04524e6955", # Parent of the merge commit (https://github.com/Rdatatable/data.table/commit/ed72e398df76a0fcfd134a4ad92356690e4210ea) of the PR (https://github.com/Rdatatable/data.table/pull/5054) that fixes the issue
-    Fast = "ed72e398df76a0fcfd134a4ad92356690e4210ea"), # Merge commit of the PR (https://github.com/Rdatatable/data.table/pull/5054) that fixes the issue
+    Fast = "ed72e398df76a0fcfd134a4ad92356690e4210ea"), # Merge commit of the PR (https://github.com/Rdatatable/data.table/pull/5054) that fixes the issue  # Test case created directly using the atime code below (not adapted from any other benchmark), based on the issue/fix PR https://github.com/Rdatatable/data.table/pull/5054#issue-930603663 "melt should be more efficient when there are missing input columns."
+
+  # Test case created from @tdhock's comment https://github.com/Rdatatable/data.table/pull/6393#issuecomment-2327396833, in turn adapted from @philippechataignon's comment https://github.com/Rdatatable/data.table/pull/6393#issuecomment-2326714012
+  "fwrite refactored in #6393" = atime::atime_test(
+    setup = {
+      set.seed(1)
+      NC = 10L
+      L <- data.table(i=1:N)
+      L[, paste0("V", 1:NC) := replicate(NC, rnorm(N), simplify=FALSE)]
+      out.csv <- tempfile()
+    },
+    expr = data.table::fwrite(L, out.csv, compress="gzip"),
+    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.
+
+  # 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)
+    tests=extra.test.list)
 # nolint end: undesirable_operator_linter.

.ci/ci.R

@@ -111,7 +111,7 @@ mirror.packages <-
 function(pkgs,
          which = c("Depends", "Imports", "LinkingTo"),
          repos = getOption("repos"),
-         type = c("source", "mac.binary", "win.binary"),
+         type = c("source", "mac.binary.big-sur-arm64", "win.binary"),
          repodir,
          except.repodir = repodir,
          except.priority = "base",
@@ -155,10 +155,11 @@ function(pkgs,
   db <- utils::available.packages(repos.url, type = type)
   allpkgs <- c(pkgs, unlist(tools::package_dependencies(unique(pkgs), db, which, recursive = TRUE), use.names = FALSE))
   except <- c("R", unlist(tools:::.get_standard_package_names()[except.priority], use.names = FALSE))
-  ## do not re-download existing packages, ignore version
+  ## do not re-download existing packages with the right version
   if (length(except.repodir) && file.exists(file.path(contrib.url(except.repodir, type = type, ver = binary.ver), "PACKAGES"))) {
     except.curl <- contrib.url(file.path("file:", normalizePath(except.repodir)), type = type, ver = binary.ver)
-    except <- c(except, rownames(utils::available.packages(except.curl, type = type, fields = "Package")))
+    except.db <- utils::available.packages(except.curl, type = type, fields = "Package")
+    except <- c(except, merge(db, except.db, by = c("Package", "Version", "MD5sum"))[,"Package"])
   }
   newpkgs <- setdiff(allpkgs, except)
   if (!all(availpkgs<-newpkgs %in% rownames(db))) {
@@ -169,10 +170,18 @@ function(pkgs,
     newpkgs <- newpkgs[availpkgs]
   }
 
-  pkgsext <- switch(type,
+  typeshort <- if (startsWith(type, "mac.binary.")) "mac.binary" else type
+  pkgsext <- switch(typeshort,
                     "source" = "tar.gz",
                     "mac.binary" = "tgz",
                     "win.binary" = "zip")
+  ## clean up stale package files for which new versions will be downloaded
+  if (file.exists(file.path(destdir, "PACKAGES"))) {
+    repo.db <- utils::available.packages(file.path("file:", normalizePath(destdir)), type = type)
+    oldver <- repo.db[repo.db[, "Package"] %in% newpkgs, c("Package", "Version"), drop=FALSE]
+    oldfiles <- file.path(destdir, sprintf("%s_%s.%s", oldver[,"Package"], oldver[,"Version"], pkgsext))
+    unlink(oldfiles[file.exists(oldfiles)])
+  }
   pkgsver <- db[db[, "Package"] %in% newpkgs, c("Package", "Version"), drop=FALSE]
   dlfiles <- file.path(destdir, sprintf("%s_%s.%s", pkgsver[,"Package"], pkgsver[,"Version"], pkgsext))
   unlink(dlfiles[file.exists(dlfiles)])
@@ -181,7 +190,7 @@ function(pkgs,
   dp <- utils::download.packages(pkgs = newpkgs, destdir = destdir,
                                  available = db, contriburl = repos.url,
                                  type = type, method = method, quiet = quiet)
-  tools::write_PACKAGES(dir = destdir, type = type, ...)
+  tools::write_PACKAGES(dir = destdir, type = typeshort, ...)
   dp
 }