Skip to content

Commit

Permalink
Gforce edge case creates segfault (#5109)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ben-schwen
ben-schwen authored Aug 24, 2021
1 parent 897ac6d commit b33dee6
Show file tree
Hide file tree
Showing 4 changed files with 220 additions and 387 deletions.
26 changes: 14 additions & 12 deletions NEWS.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
DT = data.table(A=1:3, B=letters[1:3])
DT[A>3, .(ITEM='A>3', A, B)] # (1)
DT[A>3][, .(ITEM='A>3', A, B)] # (2)
# the above are now equivalent as expected and return:
# the above are now equivalent as expected and return:
Empty data.table (0 rows and 3 cols): ITEM,A,B
# Previously, (2) returned :
ITEM A B
Expand All @@ -30,12 +30,12 @@
2: In as.data.table.list(jval, .named = NULL) :
Item 3 has 0 rows but longest item has 1; filled with NA
```

```R
DT = data.table(A=1:3, B=letters[1:3], key="A")
DT[.(1:3, double()), B]
# new result :
character(0)
character(0)
# old result :
[1] "a" "b" "c"
Warning message:
Expand All @@ -51,7 +51,7 @@
DT[, sum(colB), keyby="colA"]
DT[, sum(colB), by="colA", keyby=TRUE] # same
```

7. `fwrite()` gains a new `datatable.fwrite.sep` option to change the default separator, still `","` by default. Thanks to Tony Fischetti for the PR. As is good practice in R in general, we usually resist new global options for the reason that a user changing the option for their own code can inadvertently change the behaviour of any package using `data.table` too. However, in this case, the global option affects file output rather than code behaviour. In fact, the very reason the user may wish to change the default separator is that they know a different separator is more appropriate for their data being passed to the package using `fwrite` but cannot otherwise change the `fwrite` call within that package.

8. `melt()` now supports `NA` entries when specifying a list of `measure.vars`, which translate into runs of missing values in the output. Useful for melting wide data with some missing columns, [#4027](https://github.com/Rdatatable/data.table/issues/4027). Thanks to @vspinu for reporting, and @tdhock for implementing.
Expand Down Expand Up @@ -86,7 +86,7 @@
out_col_name = "sum_x"
)]
```

11. `DT[, if (...) .(a=1L) else .(a=1L, b=2L), by=group]` now returns a 1-column result with warning `j may not evaluate to the same number of columns for each group`, rather than error `'names' attribute [2] must be the same length as the vector`, [#4274](https://github.com/Rdatatable/data.table/issues/4274). Thanks to @robitalec for reporting, and Michael Chirico for the PR.

12. Typo checking in `i` available since 1.11.4 is extended to work in non-English sessions, [#4989](https://github.com/Rdatatable/data.table/issues/4989). Thanks to Michael Chirico for the PR.
Expand Down Expand Up @@ -114,7 +114,7 @@
```R
mtcars |> DT(mpg>20, .(mean_hp=mean(hp)), by=cyl)
```

23. `DT[i, nomatch=NULL]` where `i` contains row numbers now excludes `NA` and any outside the range [1,nrow], [#3109](https://github.com/Rdatatable/data.table/issues/3109) [#3666](https://github.com/Rdatatable/data.table/issues/3666). Before, `NA` rows were returned always for such values; i.e. `nomatch=0|NULL` was ignored. Thanks Michel Lang and Hadley Wickham for the requests, and Jan Gorecki for the PR. Using `nomatch=0` in this case when `i` is row numbers generates the warning `Please use nomatch=NULL instead of nomatch=0; see news item 5 in v1.12.0 (Jan 2019)`.

```R
Expand Down Expand Up @@ -246,26 +246,26 @@
# 2: b NA # NA because there are no non-NA, naturally
# no inconvenient warning
```

36. `DT[, min(int64Col), by=grp]` (and `max`) would return incorrect results for `bit64::integer64` columns, [#4444](https://github.com/Rdatatable/data.table/issues/4444). Thanks to @go-see for reporting, and Michael Chirico for the PR.

37. `fread(dec=',')` was able to guess `sep=','` and return an incorrect result, [#4483](https://github.com/Rdatatable/data.table/issues/4483). Thanks to Michael Chirico for reporting and fixing. It was already an error to provide both `sep=','` and `dec=','` manually.

```R
fread('A|B|C\n1|0,4|a\n2|0,5|b\n', dec=',') # no problem

# A B C
# <int> <num> <char>
# 1: 1 0.4 a
# 2: 2 0.5 b

fread('A|B,C\n1|0,4\n2|0,5\n', dec=',')

# A|B C # old result guessed sep=',' despite dec=','
# <char> <int>
# 1: 1|0 4
# 2: 2|0 5

# A B,C # now detects sep='|' correctly
# <int> <num>
# 1: 1 0.4
Expand All @@ -276,9 +276,9 @@

```
IDateTime("20171002095500", format="%Y%m%d%H%M%S")

# was :
# Error in charToDate(x) :
# Error in charToDate(x) :
# character string is not in a standard unambiguous format

# now :
Expand All @@ -287,6 +287,8 @@
# 1: 2017-10-02 09:55:00
```

39. `DT[i, sum(b), by=grp]` (and other optimized-by-group aggregates: `mean`, `var`, `sd`, `median`, `prod`, `min`, `max`, `first`, `last`, `head` and `tail`) could segfault if `i` contained row numbers and one or more were NA, [#1994](https://github.com/Rdatatable/data.table/issues/1994). Thanks to Arun Srinivasan for reporting, and Benjamin Schwendinger for the PR.

## NOTES

1. New feature 29 in v1.12.4 (Oct 2019) introduced zero-copy coercion. Our thinking is that requiring you to get the type right in the case of `0` (type double) vs `0L` (type integer) is too inconvenient for you the user. So such coercions happen in `data.table` automatically without warning. Thanks to zero-copy coercion there is no speed penalty, even when calling `set()` many times in a loop, so there's no speed penalty to warn you about either. However, we believe that assigning a character value such as `"2"` into an integer column is more likely to be a user mistake that you would like to be warned about. The type difference (character vs integer) may be the only clue that you have selected the wrong column, or typed the wrong variable to be assigned to that column. For this reason we view character to numeric-like coercion differently and will warn about it. If it is correct, then the warning is intended to nudge you to wrap the RHS with `as.<type>()` so that it is clear to readers of your code that a coercion from character to that type is intended. For example :
Expand Down
2 changes: 1 addition & 1 deletion R/test.data.table.R
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ test.data.table = function(script="tests.Rraw", verbose=FALSE, pkg=".", silent=F
catf("10 longest running tests took %ds (%d%% of %ds)\n", as.integer(tt<-DT[, sum(time)]), as.integer(100*tt/(ss<-timings[,sum(time)])), as.integer(ss))
print(DT, class=FALSE)

catf("All %d tests (last %s) in %s completed ok in %s\n", ntest, env$prevtest, names(fn), timetaken(env$started.at))
catf("All %d tests (last %.8g) in %s completed ok in %s\n", ntest, env$prevtest, names(fn), timetaken(env$started.at))

## this chunk requires to include new suggested deps: graphics, grDevices
#memtest.plot = function(.inittime) {
Expand Down
44 changes: 35 additions & 9 deletions inst/tests/tests.Rraw
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3257,7 +3257,7 @@ Sep,33.5,19.4,15.7,11.9,0,100.8,100.8,0,12.7,12.7,0,174.1")
x[ , r := as.raw(c(0, 1))]
test(1037.414, melt(x, id.vars='x1', measure.vars='r'),
error="Unknown column type 'raw' for column 'r'")

# test dispatch for non-data.table objects, #4864.
if (inherits(try(getNamespace("reshape2"), silent=TRUE),"try-error")) {
test(1038.001, melt(as.data.frame(DT), id.vars=1:2, measure.vars=5:6),
Expand Down Expand Up @@ -6759,7 +6759,7 @@ if (test_xts) {
" 6: 1970-01-07 6", " 7: 1970-01-08 7", " 8: 1970-01-09 8",
" 9: 1970-01-10 9", "10: 1970-01-11 10"))
options(old)

# as.data.table.xts(foo) had incorrect integer index with a column name called 'x', #4897
M = xts::as.xts(matrix(1, dimnames=list("2021-05-23", "x"))) # xts:: just to be extra robust; shouldn't be needed with rm(as.xts) above
test(1465.19, inherits(as.data.table(M)$index,"POSIXct"))
Expand Down Expand Up @@ -14711,9 +14711,9 @@ DT = data.table(id = c(1L,1L,2L), v = as.raw(0:2))
test(2020.01, DT[, min(v), by=id], error="'raw' not supported by GForce min/max")
test(2020.02, DT[, max(v), by=id], error="'raw' not supported by GForce min/max")
test(2020.03, DT[, median(v), by=id], error="'raw' not supported by GForce median")
test(2020.04, DT[, head(v, 1), by=id], error="'raw' not supported by GForce head")
test(2020.05, DT[, tail(v, 1), by=id], error="'raw' not supported by GForce tail")
test(2020.06, DT[, v[1], by=id], error="'raw' not supported by GForce subset")
test(2020.04, DT[, head(v, 1), by=id], error="'raw' not supported by GForce head/tail/first/last/`[`")
test(2020.05, DT[, tail(v, 1), by=id], error="'raw' not supported by GForce head/tail/first/last/`[`")
test(2020.06, DT[, v[1], by=id], error="'raw' not supported by GForce head/tail/first/last/`[`")
test(2020.07, DT[, sd(v), by=id], error="'raw' not supported by GForce sd")
test(2020.08, DT[, var(v), by=id], error="'raw' not supported by GForce var")
test(2020.09, DT[, prod(v), by=id], error="'raw' not supported by GForce prod")
Expand Down Expand Up @@ -17062,7 +17062,7 @@ registerS3method("format_col", "complex", format_col.complex)
x = data.table(z = c(1 + 3i, 2 - 1i, pi + 2.718i))
test(2130.12, x, output = '(1.0, 3.0i)')
rm(format_col.complex)
registerS3method("format_col", "complex", format_col.default)
registerS3method("format_col", "complex", format_col.default)
# otherwise it remains registered after test.data.table() and causes test 1610.1 to fail on the next run for example, and user display if they have complex data
# haven't found a way to unregister an S3 method (tried registering NULL but there's an error that NULL isn't a function)

Expand Down Expand Up @@ -17779,7 +17779,7 @@ test(2188.12, fifelse(c(TRUE, FALSE, TRUE, NA), NA, NA, as.Date("2020-01-01")),
test(2188.13, fifelse(TRUE, 1L, 2.0, "a"), error="'na' is of type character but 'no' is double. Please") # smart error message
test(2188.14, fifelse(TRUE, NA, 2, as.Date("2019-07-07")), error="'no' has different class than 'na'. Please")
test(2188.15, fifelse(TRUE, NA, factor('a'), factor('a', levels = c('a','b'))), error="'no' and 'na' are both type factor but their levels are different")
test(2188.16, fifelse(c(NA, NA), 1L, 2L, NULL), c(NA_integer_, NA_integer_)) # NULL `na` is treated as NA
test(2188.16, fifelse(c(NA, NA), 1L, 2L, NULL), c(NA_integer_, NA_integer_)) # NULL `na` is treated as NA

# rolling join expected output on non-matching join column has been fixed #1913
DT = data.table(ID=1:5, A=c(1.3, 1.7, 2.4, 0.9, 0.6))
Expand Down Expand Up @@ -17821,7 +17821,7 @@ if (test_bit64) {
DT[a==1, a:=12]
DT[a==2, a:=as.integer64(13)]
test(2193.1, DT, data.table(a = as.integer64(c(12,13,3:10))))

# X[Y,,by=.EACHI] when Y contains integer64 also fixed in 1.12.4, #3779
X = data.table(x=1:3)
Y = data.table(x=1:2, y=as.integer64(c(10,20)))
Expand Down Expand Up @@ -17899,7 +17899,7 @@ setDTthreads() # restore default throttle
# fwrite now allows sep="", #4817
test(2202.1, fwrite(data.frame(a="id", b=letters[1:5], c=1:5), sep=""),
output = c("abc", paste0("id", letters[1:5], 1:5)))
test(2202.2, fwrite(data.frame(a="id", b=1:1e2), sep=""),
test(2202.2, fwrite(data.frame(a="id", b=1:1e2), sep=""),
output = c("ab", paste0("id", 1:1e2)))
test(2202.3, fwrite(data.table(a=c(NA, 2, 3.01), b=c('foo', NA, 'bar')), sep=""),
output=c("ab", "foo", "2", "3.01bar"))
Expand Down Expand Up @@ -18009,3 +18009,29 @@ test(2210.24, DT[-c(1L,0L)], data.table(x=2:4)) # codecov gap, not related to no
test(2210.25, DT[-c(1L,0L), nomatch=NULL], data.table(x=2:4))
test(2210.26, DT[-c(1L,0L), nomatch=0], data.table(x=2:4), warning="Please use nomatch=NULL")

# NA in i would segfault gforce, #1994
DT = data.table(a=1L, b=2, c="a", grp=1L)
i = c(1L,NA,NA,NA) # 3 NA to trigger segfault in var (min 3 obs) otherwise just c(1L,NA) is enough to trigger the others
funs = c("sum","mean","var","sd","median","prod","min","max","`[`","first","last","head","tail")
EVAL = function(...) {
e = paste0(...)
# cat(e,"\n") # uncomment to check the queries tested
eval(parse(text=e))
}
testnum = 2211.0
for (col in c("a","b","c")) {
testnum = testnum+0.1
for (fi in seq_along(funs)) {
if (col=="c" && fi<=6L) next # first 6 funs don't support type character
f = funs[fi]
testnum = testnum+0.001
test(testnum, EVAL("DT[i, ",f,"(",col, if(fi>8L)", 1L","), by=grp]"), # segfault before when NA in i
EVAL("DT[i][, ",f,"(",col, if(fi>8L)", 1L","), by=grp]")) # ok before by taking DT[i] subset first
if (fi<=8L) {
testnum = testnum+0.001
test(testnum, EVAL("DT[i, ",f,"(",col,", na.rm=TRUE), by=grp]"),
EVAL("DT[i][, ",f,"(",col,", na.rm=TRUE), by=grp]"))
}
}
}

Loading

0 comments on commit b33dee6

Please sign in to comment.