diff --git a/.ci/atime/tests.R b/.ci/atime/tests.R index fddfb1347b..816a7621eb 100644 --- a/.ci/atime/tests.R +++ b/.ci/atime/tests.R @@ -17,66 +17,41 @@ # @note Please check https://github.com/tdhock/atime/blob/main/vignettes/data.table.Rmd for more information. # nolint start: undesirable_operator_linter. ':::' needed+appropriate here. test.list <- atime::atime_test_list( - # A function to customize R package metadata and source files to facilitate version-specific installation and testing. - # - # This is specifically tailored for handling data.table which requires specific changes in non-standard files (such as the object file name in Makevars and version checking code in onLoad.R) - # to support testing across different versions (base and HEAD for PRs, commits associated with historical regressions, etc.) of the package. - # It appends a SHA1 hash to the package name (PKG.SHA), ensuring each version can be installed and tested separately. - # - # @param old.Package Current name of the package. - # @param new.Package New name of the package, including a SHA hash. - # @param sha SHA1 hash used for differentiating versions. - # @param new.pkg.path Path to the package files. - # - # @details - # The function modifies: - # - DESCRIPTION, updating the package name. - # - Makevars, customizing the shared object file name and adjusting the build settings. - # - R/onLoad.R, adapting custom version checking for package loading operations. - # - NAMESPACE, changing namespace settings for dynamic linking. - # - # @examples - # pkg.edit.fun("data.table", "data.table.some_SHA1_hash", "some_SHA1_hash", "/path/to/data.table") - # - # @return None (performs in-place file modifications) - # @note This setup is typically unnecessary for most packages but essential for data.table due to its unique configuration. pkg.edit.fun = function(old.Package, new.Package, sha, new.pkg.path) { - pkg_find_replace <- function(glob, FIND, REPLACE) { - atime::glob_find_replace(file.path(new.pkg.path, glob), FIND, REPLACE) - } - Package_regex <- gsub(".", "_?", old.Package, fixed = TRUE) - Package_ <- gsub(".", "_", old.Package, fixed = TRUE) - new.Package_ <- paste0(Package_, "_", sha) - pkg_find_replace( - "DESCRIPTION", - paste0("Package:\\s+", old.Package), - paste("Package:", new.Package)) - pkg_find_replace( - file.path("src", "Makevars.*in"), - Package_regex, - new.Package_) - pkg_find_replace( - file.path("R", "onLoad.R"), - Package_regex, - new.Package_) - pkg_find_replace( - file.path("R", "onLoad.R"), - sprintf('packageVersion\\("%s"\\)', old.Package), - sprintf('packageVersion\\("%s"\\)', new.Package)) - pkg_find_replace( - file.path("src", "init.c"), - paste0("R_init_", Package_regex), - paste0("R_init_", gsub("[.]", "_", new.Package_))) - pkg_find_replace( - "NAMESPACE", - sprintf('useDynLib\\("?%s"?', Package_regex), - paste0('useDynLib(', new.Package_)) - }, - - # Performance regression discussed in: https://github.com/Rdatatable/data.table/issues/4311 - # Fixed in: https://github.com/Rdatatable/data.table/pull/4440 + pkg_find_replace <- function(glob, FIND, REPLACE) { + atime::glob_find_replace(file.path(new.pkg.path, glob), FIND, REPLACE) + } + Package_regex <- gsub(".", "_?", old.Package, fixed = TRUE) + Package_ <- gsub(".", "_", old.Package, fixed = TRUE) + new.Package_ <- paste0(Package_, "_", sha) + pkg_find_replace( + "DESCRIPTION", + paste0("Package:\\s+", old.Package), + paste("Package:", new.Package)) + pkg_find_replace( + file.path("src", "Makevars.*in"), + Package_regex, + new.Package_) + pkg_find_replace( + file.path("R", "onLoad.R"), + Package_regex, + new.Package_) + pkg_find_replace( + file.path("R", "onLoad.R"), + sprintf('packageVersion\\("%s"\\)', old.Package), + sprintf('packageVersion\\("%s"\\)', new.Package)) + pkg_find_replace( + file.path("src", "init.c"), + paste0("R_init_", Package_regex), + paste0("R_init_", gsub("[.]", "_", new.Package_))) + pkg_find_replace( + "NAMESPACE", + sprintf('useDynLib\\("?%s"?', Package_regex), + paste0('useDynLib(', new.Package_)) + }, + "shallow regression fixed in #4440" = atime::atime_test( - N = 10^seq(3,8), + N = 10^seq(3, 8), setup = { set.seed(1L) dt <- data.table(a = sample.int(N)) @@ -119,6 +94,16 @@ test.list <- atime::atime_test_list( data.table:::setDT(L) }, Slow = "c4a2085e35689a108d67dacb2f8261e4964d7e12", # Parent of the first commit in the PR that fixes the issue (https://github.com/Rdatatable/data.table/commit/7cc4da4c1c8e568f655ab5167922dcdb75953801) - Fast = "1872f473b20fdcddc5c1b35d79fe9229cd9a1d15") # Last commit in the PR that fixes the issue (https://github.com/Rdatatable/data.table/pull/5427/commits) + Fast = "1872f473b20fdcddc5c1b35d79fe9229cd9a1d15"), # Last commit in the PR that fixes the issue (https://github.com/Rdatatable/data.table/pull/5427/commits) for my fork here. + # Merge commit in the PR that fixes the issue (https://github.com/Rdatatable/data.table/pull/5427) # <-- This too would probably work but in my fork I don't have any commits beyond 1872f473b20fdcddc5c1b35d79fe9229cd9a1d15 for that branch, + + # Issue reported in: https://github.com/Rdatatable/data.table/issues/6286 + "by with verbose = TRUE" = atime::atime_test( + N = 10^seq(1, 8), + setup = { + dt = data.table(a = 1:N) + }, + expr = data.table:::copy(dt)[, 1, by = a, verbose = TRUE], + "Slow" = "a01f00f7438daf4612280d6886e6929fa8c8f76e", + "Fast" = "cd497408bb4dc6650d871b5076e738420ff431d7") ) -# nolint end: undesirable_operator_linter. diff --git a/.github/workflows/autocomment.yml b/.github/workflows/autocomment.yml new file mode 100644 index 0000000000..0f9a2b32cb --- /dev/null +++ b/.github/workflows/autocomment.yml @@ -0,0 +1,21 @@ +name: Autocomment atime-based performance regression analysis on PRs + +on: + pull_request: + branches: + - '*' + types: + - opened + - reopened + - synchronize + +jobs: + comment: + runs-on: ubuntu-latest + container: ghcr.io/iterative/cml:0-dvc2-base1 + env: + GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }} + repo_token: ${{ secrets.GITHUB_TOKEN }} + R_KEEP_PKG_SOURCE: yes + steps: + - uses: Anirban166/Autocomment-atime-results@v1.1.0 diff --git a/src/negate.c b/src/negate.c index 4db3767ff8..2e393d445f 100644 --- a/src/negate.c +++ b/src/negate.c @@ -11,7 +11,6 @@ void negateByRef(SEXP x) { } } - SEXP notchin(SEXP x, SEXP table) { // see discussion in PR#4931 SEXP result = PROTECT(chin(x, table));