Skip to content

Commit

Permalink
remove quote >
Browse files Browse the repository at this point in the history
  • Loading branch information
Toby Dylan Hocking committed Oct 4, 2024
2 parents 20bbf21 + b4538a0 commit ff2d504
Show file tree
Hide file tree
Showing 79 changed files with 11,728 additions and 540 deletions.
110 changes: 74 additions & 36 deletions .ci/atime/tests.R
Original file line number Diff line number Diff line change
@@ -1,3 +1,24 @@
# 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(
"colClasses=list(Date='date')",
"colClasses='Date'",
"select=list(Date='date')")
extra.test.list <- list()
for (extra.arg in extra.args.6107){
this.test <- atime::atime_test(
setup = {
set.seed(1)
DT = data.table(date=.Date(sample(20000, N, replace=TRUE)))
tmp_csv = tempfile()
fwrite(DT, tmp_csv)
},
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
}

# A list of performance tests.
#
# See documentation in https://github.com/Rdatatable/data.table/wiki/Performance-testing for best practices.
Expand All @@ -19,6 +40,8 @@
# @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(
# Common N and pkg.edit.fun are defined here, and inherited in all test cases below which do not re-define them.
N = as.integer(10^seq(1, 7, by=0.25)),
# 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)
Expand Down Expand Up @@ -75,10 +98,9 @@ test.list <- atime::atime_test_list(
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
# 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(
N = 10^seq(3,8),
setup = {
set.seed(1L)
dt <- data.table(a = sample.int(N))
Expand All @@ -89,29 +111,27 @@ test.list <- atime::atime_test_list(
Regression = "b1b1832b0d2d4032b46477d9fe6efb15006664f4", # Parent of the first commit (https://github.com/Rdatatable/data.table/commit/0f0e7127b880df8459b0ed064dc841acd22f5b73) in the PR (https://github.com/Rdatatable/data.table/pull/4440/commits) that fixes the regression
Fixed = "9d3b9202fddb980345025a4f6ac451ed26a423be"), # Merge commit in the PR that fixed the regression (https://github.com/Rdatatable/data.table/pull/4440)

# Test based on: https://github.com/Rdatatable/data.table/issues/5424
# Performance regression introduced from a commit in: https://github.com/Rdatatable/data.table/pull/4491
# Fixed in: https://github.com/Rdatatable/data.table/pull/5463
# Test based on https://github.com/Rdatatable/data.table/issues/5424
# Performance regression introduced from a commit in https://github.com/Rdatatable/data.table/pull/4491
# Test case adapted from https://github.com/Rdatatable/data.table/pull/5463#issue-1373642456 which is the fix PR.
"memrecycle regression fixed in #5463" = atime::atime_test(
N = 10^seq(3, 8),
setup = {
n <- N/100
bigN <- N*100
set.seed(2L)
dt <- data.table(
g = sample(seq_len(n), N, TRUE),
x = runif(N),
g = sample(seq_len(N), bigN, TRUE),
x = runif(bigN),
key = "g")
dt_mod <- copy(dt)
},
expr = data.table:::`[.data.table`(dt_mod, , N := .N, by = g),
Before = "be2f72e6f5c90622fe72e1c315ca05769a9dc854", # Parent of the regression causing commit (https://github.com/Rdatatable/data.table/commit/e793f53466d99f86e70fc2611b708ae8c601a451) in the PR that introduced the issue (https://github.com/Rdatatable/data.table/pull/4491/commits)
Regression = "e793f53466d99f86e70fc2611b708ae8c601a451", # Commit responsible for regression in the PR that introduced the issue (https://github.com/Rdatatable/data.table/pull/4491/commits)
Fixed = "58409197426ced4714af842650b0cc3b9e2cb842"), # Last commit in the PR that fixed the regression (https://github.com/Rdatatable/data.table/pull/5463/commits)
Before = "be2f72e6f5c90622fe72e1c315ca05769a9dc854", # Parent of the regression causing commit (https://github.com/Rdatatable/data.table/commit/e793f53466d99f86e70fc2611b708ae8c601a451) in the PR (https://github.com/Rdatatable/data.table/pull/4491/commits) that introduced the issue
Regression = "e793f53466d99f86e70fc2611b708ae8c601a451", # Commit responsible for regression in the PR (https://github.com/Rdatatable/data.table/pull/4491/commits) that introduced the issue
Fixed = "58409197426ced4714af842650b0cc3b9e2cb842"), # Last commit in the PR (https://github.com/Rdatatable/data.table/pull/5463/commits) that fixed the regression

# Issue reported in: https://github.com/Rdatatable/data.table/issues/5426
# To be fixed in: https://github.com/Rdatatable/data.table/pull/5427
# Issue reported in https://github.com/Rdatatable/data.table/issues/5426
# Test case adapted from https://github.com/Rdatatable/data.table/pull/5427#issue-1323678063 which is the fix PR.
"setDT improved in #5427" = atime::atime_test(
N = 10^seq(1, 7),
setup = {
L <- replicate(N, 1, simplify = FALSE)
setDT(L)
Expand All @@ -120,48 +140,41 @@ test.list <- atime::atime_test_list(
data.table:::setattr(L, "class", NULL)
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)
Slow = "c4a2085e35689a108d67dacb2f8261e4964d7e12", # Parent of the first commit (https://github.com/Rdatatable/data.table/commit/7cc4da4c1c8e568f655ab5167922dcdb75953801) in the PR (https://github.com/Rdatatable/data.table/pull/5427/commits) that fixes the issue
Fast = "af48a805e7a5026a0c2d0a7fd9b587fea5cfa3c4"), # Last commit in the PR (https://github.com/Rdatatable/data.table/pull/5427/commits) that fixes the issue

# Issue reported in: https://github.com/Rdatatable/data.table/issues/4200
# To be fixed in: https://github.com/Rdatatable/data.table/pull/4558
# Test case adapted from https://github.com/Rdatatable/data.table/issues/4200#issuecomment-645980224 which is where the issue was reported.
# Fixed in https://github.com/Rdatatable/data.table/pull/4558
"DT[by] fixed in #4558" = atime::atime_test(
N = 10^seq(1, 20),
setup = {
d <- data.table(
id3 = sample(c(seq.int(N*0.9), sample( N*0.9, N*0.1, TRUE))),
id = sample(c(seq.int(N * 0.9), sample(N * 0.9, N * 0.1, TRUE))),
v1 = sample(5L, N, TRUE),
v2 = sample(5L, N, TRUE)
)
},
expr = {
expr=data.table:::`[.data.table`(d, , max(v1) - min(v2), by = id3)
},
expr = data.table:::`[.data.table`(d, , max(v1) - min(v2), by = id),
Before = "7a9eaf62ede487625200981018d8692be8c6f134", # Parent of the first commit (https://github.com/Rdatatable/data.table/commit/515de90a6068911a148e54343a3503043b8bb87c) in the PR (https://github.com/Rdatatable/data.table/pull/4164/commits) that introduced the regression
Regression = "c152ced0e5799acee1589910c69c1a2c6586b95d", # Parent of the first commit (https://github.com/Rdatatable/data.table/commit/15f0598b9828d3af2eb8ddc9b38e0356f42afe4f) in the PR (https://github.com/Rdatatable/data.table/pull/4558/commits) that fixes the regression
Fixed = "f750448a2efcd258b3aba57136ee6a95ce56b302"), # Second commit of the PR (https://github.com/Rdatatable/data.table/pull/4558/commits) that fixes the regression

# Issue with sorting again when already sorted: https://github.com/Rdatatable/data.table/issues/4498
# Fixed in: https://github.com/Rdatatable/data.table/pull/4501
# Issue with sorting again when already sorted, as reported in https://github.com/Rdatatable/data.table/issues/4498
# Test case adapted from https://github.com/Rdatatable/data.table/pull/4501#issue-625311918 which is the fix PR.
"DT[,.SD] improved in #4501" = atime::atime_test(
N = 10^seq(1, 10, by=0.5),
setup = {
set.seed(1)
L = as.data.table(as.character(rnorm(N, 1, 0.5)))
setkey(L, V1)
},
## New DT can safely retain key.
expr = {
data.table:::`[.data.table`(L, , .SD)
},
expr = data.table:::`[.data.table`(L, , .SD),
Fast = "353dc7a6b66563b61e44b2fa0d7b73a0f97ca461", # Close-to-last merge commit in the PR (https://github.com/Rdatatable/data.table/pull/4501/commits) that fixes the issue
Slow = "3ca83738d70d5597d9e168077f3768e32569c790", # Circa 2024 master parent of close-to-last merge commit (https://github.com/Rdatatable/data.table/commit/353dc7a6b66563b61e44b2fa0d7b73a0f97ca461) in the PR (https://github.com/Rdatatable/data.table/pull/4501/commits) that fixes the issue
Slower = "cacdc92df71b777369a217b6c902c687cf35a70d"), # Circa 2020 parent of the first commit (https://github.com/Rdatatable/data.table/commit/74636333d7da965a11dad04c322c752a409db098) in the PR (https://github.com/Rdatatable/data.table/pull/4501/commits) that fixes the issue

# Issue reported in: https://github.com/Rdatatable/data.table/issues/6286
# Fixed in: https://github.com/Rdatatable/data.table/pull/6296
"DT[by, verbose = TRUE] improved in #6296" = atime::atime_test(
N = 10^seq(1, 9),
# Test case adapted from https://github.com/Rdatatable/data.table/issues/6286#issue-2412141289 which is where the issue was reported.
# Fixed in https://github.com/Rdatatable/data.table/pull/6296
"DT[by,verbose=TRUE] improved in #6296" = atime::atime_test(
setup = {
dt = data.table(a = 1:N)
dt_mod <- copy(dt)
Expand All @@ -170,5 +183,30 @@ test.list <- atime::atime_test_list(
Slow = "a01f00f7438daf4612280d6886e6929fa8c8f76e", # Parent of the first commit (https://github.com/Rdatatable/data.table/commit/fc0c1e76408c34a8482f16f7421d262c7f1bde32) in the PR (https://github.com/Rdatatable/data.table/pull/6296/commits) that fixes the issue
Fast = "f248bbe6d1204dfc8def62328788eaadcc8e17a1"), # Merge commit of the PR (https://github.com/Rdatatable/data.table/pull/6296) that fixes the issue

NULL)
# Test case adapted from https://github.com/Rdatatable/data.table/issues/5492#issue-1416598382 which is where the issue was reported,
# and from https://github.com/Rdatatable/data.table/pull/5493#issue-1416656788 which is the fix PR.
"transform improved in #5493" = atime::atime_test(
setup = {
df <- data.frame(x = runif(N))
dt <- as.data.table(df)
},
expr = data.table:::transform.data.table(dt, y = round(x)),
Slow = "0895fa247afcf6b38044bd5f56c0d209691ddb31", # Parent of the first commit (https://github.com/Rdatatable/data.table/commit/93ce3ce1373bf733ebd2036e2883d2ffe377ab58) in the PR (https://github.com/Rdatatable/data.table/pull/5493/commits) that fixes the issue
Fast = "2d1a0575f87cc50e90f64825c30d7a6cb6b05dd7"), # Merge commit of the PR (https://github.com/Rdatatable/data.table/pull/5493) 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."
"melt improved in #5054" = atime::atime_test(
setup = {
DT <- as.data.table(as.list(1:N))
measure.vars <- lapply(1:N, function(i) {
x = rep(NA, N)
x[i] = i
x
})
},
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

tests=extra.test.list)
# nolint end: undesirable_operator_linter.
5 changes: 4 additions & 1 deletion .ci/linters/po/msgfmt_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ msgfmt_linter <- function(po_file) {
stop(po_file, " has not been compiled as ", mo_ref, ". Please fix.")
}
if (tools::md5sum(mo_ref) == tools::md5sum(mo_tmp)) return(invisible())
# TODO(#6517): Re-activate this part of the check to ensure .mo is up to date.
cat(sprintf("Note: MD5 sum of msgfmt output for %s does not match %s.\n", po_file, mo_ref))
return(invisible())

# NB: file.mtime() will probably be wrong, it will reflect the check-out time of the git repo.
last_edit_time = system2("git",
Expand All @@ -33,7 +36,7 @@ msgfmt_linter <- function(po_file) {

unmo_tmp = tempfile()
unmo_ref = tempfile()
on.exit(unlink(c(unmo_tmp, unmo_ref), add=TRUE))
on.exit(unlink(c(unmo_tmp, unmo_ref)), add=TRUE)
system2("msgunfmt", c(mo_tmp, "-o", unmo_tmp))
system2("msgunfmt", c(mo_ref, "-o", unmo_ref))
cat("Here are the observed differences after converting back to .po:\n\n")
Expand Down
10 changes: 10 additions & 0 deletions .ci/linters/r/class1_linter.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
class1_linter = lintr::make_linter_from_xpath(
"
//OP-LEFT-BRACKET[
preceding-sibling::expr/expr/SYMBOL_FUNCTION_CALL[text() = 'class']
and following-sibling::expr/NUM_CONST[text() = '1' or text() = '1L']
]
/parent::expr
",
"Use class1(x) to get class(x)[1L], or classes1(x) to do so for a full list/data.table"
)
5 changes: 1 addition & 4 deletions .github/workflows/code-quality.yaml
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
on:
push:
branches:
- master
branches: [master]
pull_request:
branches:
- master

name: code-quality

Expand Down
7 changes: 3 additions & 4 deletions .github/workflows/performance-tests.yml
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
name: Autocomment atime-based performance regression analysis on PRs
name: atime performance tests

on:
pull_request:
branches:
- '*'
types:
- opened
- reopened
Expand All @@ -15,10 +13,11 @@ on:

jobs:
comment:
if: github.repository == 'Rdatatable/data.table'
runs-on: ubuntu-latest
container: ghcr.io/iterative/cml:0-dvc2-base1
env:
GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
repo_token: ${{ secrets.GITHUB_TOKEN }}
steps:
- uses: Anirban166/Autocomment-atime-results@v1.2.0
- uses: Anirban166/Autocomment-atime-results@v1.3.1
3 changes: 1 addition & 2 deletions .github/workflows/rchk.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@
# under the License.
on:
push:
branches:
- master
branches: [master]
pull_request:

name: 'rchk'
Expand Down
54 changes: 54 additions & 0 deletions .github/workflows/test-coverage.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# Workflow derived from https://github.com/r-lib/actions/tree/v2/examples
# Need help debugging build failures? Start at https://github.com/r-lib/actions#where-to-find-help
on:
push:
branches: [master]
pull_request:

name: test-coverage.yaml

permissions: read-all

jobs:
test-coverage:
runs-on: ubuntu-latest
env:
GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}

steps:
- uses: actions/checkout@v4

- uses: r-lib/actions/setup-r@v2
with:
use-public-rspm: true
r-version: '4.3' # TODO(r-lib/covr#567): Go back to using r-devel

- uses: r-lib/actions/setup-r-dependencies@v2
with:
extra-packages: any::covr, any::xml2
needs: coverage

- name: Test coverage
run: |
cov <- covr::package_coverage(
quiet = FALSE,
clean = FALSE,
install_path = file.path(normalizePath(Sys.getenv("RUNNER_TEMP"), winslash = "/"), "package")
)
covr::to_cobertura(cov)
shell: Rscript {0}

- uses: codecov/codecov-action@v4
with:
fail_ci_if_error: ${{ github.event_name != 'pull_request' && true || false }}
file: ./cobertura.xml
plugin: noop
disable_search: true
token: ${{ secrets.CODECOV_TOKEN }}

- name: Upload test results
if: failure()
uses: actions/upload-artifact@v4
with:
name: coverage-test-failures
path: ${{ runner.temp }}/package
20 changes: 10 additions & 10 deletions .gitlab-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,21 @@ variables:
_R_CHECK_FORCE_SUGGESTS_: "false"
_R_CHECK_NO_STOP_ON_TEST_ERROR_: "true"
_R_CHECK_SYSTEM_CLOCK_: "false" ## https://stackoverflow.com/questions/63613301/r-cmd-check-note-unable-to-verify-current-time
_R_CHECK_TESTS_NLINES_: "0"
TZ: "UTC" ## to avoid 'Failed to create bus connection' from timedatectl via Sys.timezone() on Docker with R 3.4.
## Setting TZ for all GLCI jobs to isolate them from timezone. We could have a new GLCI job to test under
## a non-UTC timezone, although, that's what we do routinely in dev.
R_REL_VERSION: "4.3"
R_REL_WIN_BIN: "https://cloud.r-project.org/bin/windows/base/old/4.3.2/R-4.3.2-win.exe"
RTOOLS_REL_BIN: "https://cloud.r-project.org/bin/windows/Rtools/rtools43/files/rtools43-5958-5975.exe"
R_REL_VERSION: "4.4"
R_REL_WIN_BIN: "https://cloud.r-project.org/bin/windows/base/old/4.4.1/R-4.4.1-win.exe"
RTOOLS_REL_BIN: "https://cloud.r-project.org/bin/windows/Rtools/rtools44/files/rtools44-6104-6039.exe"
RTOOLS43_HOME: "/c/rtools"
R_DEV_VERSION: "4.4"
R_DEV_VERSION: "4.5"
R_DEV_WIN_BIN: "https://cloud.r-project.org/bin/windows/base/R-devel-win.exe"
RTOOLS_DEV_BIN: "https://cloud.r-project.org/bin/windows/Rtools/rtools43/files/rtools43-5958-5975.exe"
RTOOLS44_HOME: "" ## in case R-devel will use new Rtools toolchain, now it uses 4.3 env var
R_OLD_VERSION: "4.2"
R_OLD_WIN_BIN: "https://cloud.r-project.org/bin/windows/base/old/4.2.3/R-4.2.3-win.exe"
RTOOLS_OLD_BIN: "https://cloud.r-project.org/bin/windows/Rtools/rtools42/files/rtools42-5355-5357.exe"
RTOOLS_DEV_BIN: "https://cloud.r-project.org/bin/windows/Rtools/rtools44/files/rtools44-6104-6039.exe"
RTOOLS44_HOME: "" ## in case R-devel will use new Rtools toolchain, now it uses 4.4 env var
R_OLD_VERSION: "4.3"
R_OLD_WIN_BIN: "https://cloud.r-project.org/bin/windows/base/old/4.3.3/R-4.3.3-win.exe"
RTOOLS_OLD_BIN: "https://cloud.r-project.org/bin/windows/Rtools/rtools43/files/rtools43-5958-5975.exe"
RTOOLS42_HOME: "/c/rtools"

stages:
Expand Down Expand Up @@ -114,7 +115,6 @@ test-lin-rel:
_R_CHECK_CRAN_INCOMING_: "FALSE"
_R_CHECK_CRAN_INCOMING_REMOTE_: "FALSE"
_R_CHECK_FORCE_SUGGESTS_: "TRUE"
_R_CHECK_TESTS_NLINES_: "0"
OPENBLAS_MAIN_FREE: "1"
script:
- *install-deps
Expand Down
4 changes: 4 additions & 0 deletions CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@
/src/shift.c @ben-schwen @michaelchirico
/man/shift.Rd @ben-schwen @michaelchirico

# rbind
/src/rbindlist.c @ben-schwen
/man/rbindlist.Rd @ben-schwen

# translations
/inst/po/ @michaelchirico
/po/ @michaelchirico
Expand Down
4 changes: 3 additions & 1 deletion DESCRIPTION
Original file line number Diff line number Diff line change
Expand Up @@ -89,5 +89,7 @@ Authors@R: c(
person("Iago", "Giné-Vázquez", role="ctb"),
person("Anirban", "Chetia", role="ctb"),
person("Doris", "Amoakohene", role="ctb"),
person("Ivan", "Krylov", role="ctb")
person("Ivan", "Krylov", role="ctb"),
person("Michael","Young", role="ctb"),
person("Mark", "Seeto", role="ctb")
)
1 change: 1 addition & 0 deletions NAMESPACE
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ exportClasses(data.table, IDate, ITime)
##

export(data.table, tables, setkey, setkeyv, key, "key<-", haskey, CJ, SJ, copy)
export(rowwiseDT)
export(setindex, setindexv, indices)
export(as.data.table,is.data.table,test.data.table)
export(last,first,like,"%like%","%ilike%","%flike%","%plike%",between,"%between%",inrange,"%inrange%", "%notin%")
Expand Down
Loading

0 comments on commit ff2d504

Please sign in to comment.