Skip to content

fix shift issue when n character #3355

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattdowle merged 3 commits into from
Feb 8, 2019
Merged

fix shift issue when n character #3355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a8717a
fix shift RStudio issue, closes #3354
jangorecki Feb 5, 2019
f43e854
Added news item
mattdowle Feb 8, 2019
03856e5
removed commented line. previous commit raised many deep INTEGER(k) a…
mattdowle Feb 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

1. `rbindlist()` of a malformed factor missing levels attribute is now a helpful error rather than a cryptic error about `STRING_ELT`, [#3315](https://github.com/Rdatatable/data.table/issues/3315). Thanks to Michael Chirico for reporting.

2. Forgetting `type=` in `shift(val, "lead")` would segfault, [#3354](https://github.com/Rdatatable/data.table/issues/3354). A helpful error is now produced to indicate `"lead"` is being passed to `n=` rather than the intended `type=` argument. Thanks to @SymbolixAU for reporting.

#### NOTES

1. When upgrading to 1.12.0 some Windows users might have seen `CdllVersion not found` in some circumstances. We found a way to catch that so the [helpful message](https://twitter.com/MattDowle/status/1084528873549705217) now occurs for those upgrading from versions prior to 1.12.0 too, as well as those upgrading from 1.12.0 to a later version. See item 1 in notes section of 1.12.0 below for more background.
Expand Down
1 change: 1 addition & 0 deletions R/shift.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
shift <- function(x, n=1L, fill=NA, type=c("lag", "lead", "shift"), give.names=FALSE) {
type = match.arg(type)
stopifnot(is.numeric(n))
ans = .Call(Cshift, x, as.integer(n), fill, type)
if (give.names && is.list(ans)) {
if (is.null(names(x))) {
Expand Down
5 changes: 5 additions & 0 deletions inst/tests/tests.Rraw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13304,6 +13304,11 @@ test(1979, DT[,.(a,b,if(FALSE)c)], DT[,c("a","b")])
x <- as.array(1:5)
test(1980, names(data.table(x)), "x")

# this was crashing RStudio #3354
DT = data.table( id = 1:5 , val = letters[1:5] )
test(1981.1, DT[, new_col := shift(val, "lead")], error="is.numeric(n) is not TRUE")
test(1981.2, DT[, new_col := shift(val, NA_integer_)], error="Item 1 of n is NA")


###################################
# Add new tests above this line #
Expand Down
66 changes: 33 additions & 33 deletions src/shift.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
SEXP shift(SEXP obj, SEXP k, SEXP fill, SEXP type) {

size_t size;
R_len_t i=0, j, m, nx, nk, xrows, thisk, protecti=0;
int protecti=0;
SEXP x, tmp=R_NilValue, elem, ans, thisfill, klass;
unsigned long long *dthisfill;
enum {LAG, LEAD/*, SHIFT, CYCLIC*/} stype = LAG; // currently SHIFT maps to LAG and CYCLIC is unimplemented (see comments in #1708)
Expand All @@ -16,48 +16,48 @@ SEXP shift(SEXP obj, SEXP k, SEXP fill, SEXP type) {
} else x = obj;
if (!isNewList(x))
error("x must be a list, data.frame or data.table");
if (!isInteger(k))
error("Internal error: n must be integer"); // # nocov
if (length(fill) != 1)
error("fill must be a vector of length 1");
// the following two errors should be caught by match.arg() at the R level
if (!isString(type) || length(type) != 1)
error("Internal error: invalid type for shift(), should have been caught before. please report to data.table issue tracker"); // # nocov

if (!strcmp(CHAR(STRING_ELT(type, 0)), "lag")) stype = LAG;
else if (!strcmp(CHAR(STRING_ELT(type, 0)), "lead")) stype = LEAD;
else if (!strcmp(CHAR(STRING_ELT(type, 0)), "shift")) stype = LAG; // when we get rid of nested if branches we can use SHIFT, for now it maps to LAG
else error("Internal error: invalid type for shift(), should have been caught before. please report to data.table issue tracker"); // # nocov

nx = length(x); nk = length(k);
int nx = length(x), nk = length(k);
if (!isInteger(k)) error("Internal error: k must be integer"); // # nocov
const int *kd = INTEGER(k);
for (int i=0; i<nk; i++) if (kd[i]==NA_INTEGER) error("Item %d of n is NA", i+1); // NA crashed (#3354); n is called k at C level

ans = PROTECT(allocVector(VECSXP, nk * nx)); protecti++;
for (i=0; i<nx; i++) {
for (int i=0; i<nx; i++) {
elem = VECTOR_ELT(x, i);
size = SIZEOF(elem);
xrows = length(elem);
R_len_t xrows = length(elem);
switch (TYPEOF(elem)) {
case INTSXP :
thisfill = PROTECT(coerceVector(fill, INTSXP)); protecti++;
for (j=0; j<nk; j++) {
thisk = (INTEGER(k)[j] >= 0) ? INTEGER(k)[j] : -INTEGER(k)[j];
for (int j=0; j<nk; j++) {
int thisk = (kd[j] >= 0) ? kd[j] : -kd[j];
thisk = (xrows >= thisk) ? thisk : xrows;
SET_VECTOR_ELT(ans, i*nk+j, tmp=allocVector(INTSXP, xrows) );
// LAG when type = 'lag' and n >= 0 _or_ type = 'lead' and n < 0
if ((stype == LAG && INTEGER(k)[j] >= 0) || (stype == LEAD && INTEGER(k)[j] < 0)) {
if ((stype == LAG && kd[j] >= 0) || (stype == LEAD && kd[j] < 0)) {
if (xrows - thisk > 0)
memmove((char *)DATAPTR(tmp)+(thisk*size),
(char *)DATAPTR(elem),
(xrows-thisk)*size);
for (m=0; m<thisk; m++)
for (int m=0; m<thisk; m++)
INTEGER(tmp)[m] = INTEGER(thisfill)[0];
// only two possibilities left: type = 'lead', n>=0 _or_ type = 'lag', n<0
} else {
if (xrows - thisk > 0)
memmove((char *)DATAPTR(tmp),
(char *)DATAPTR(elem)+(thisk*size),
(xrows-thisk)*size);
for (m=xrows-thisk; m<xrows; m++)
for (int m=xrows-thisk; m<xrows; m++)
INTEGER(tmp)[m] = INTEGER(thisfill)[0];
}
copyMostAttrib(elem, tmp);
Expand All @@ -77,25 +77,25 @@ SEXP shift(SEXP obj, SEXP k, SEXP fill, SEXP type) {
} else {
thisfill = PROTECT(coerceVector(fill, REALSXP)); protecti++;
}
for (j=0; j<nk; j++) {
thisk = (INTEGER(k)[j] >= 0) ? INTEGER(k)[j] : -INTEGER(k)[j];
thisk = (xrows >= INTEGER(k)[j]) ? INTEGER(k)[j] : xrows;
for (int j=0; j<nk; j++) {
int thisk = (kd[j] >= 0) ? kd[j] : -kd[j];
thisk = (xrows >= kd[j]) ? kd[j] : xrows;
SET_VECTOR_ELT(ans, i*nk+j, tmp=allocVector(REALSXP, xrows) );
if ((stype == LAG && INTEGER(k)[j] >= 0) || (stype == LEAD && INTEGER(k)[j] < 0)) {
if ((stype == LAG && kd[j] >= 0) || (stype == LEAD && kd[j] < 0)) {
if (xrows - thisk > 0) {
memmove((char *)DATAPTR(tmp)+(thisk*size),
(char *)DATAPTR(elem),
(xrows-thisk)*size);
}
for (m=0; m<thisk; m++) {
for (int m=0; m<thisk; m++) {
REAL(tmp)[m] = REAL(thisfill)[0];
}
} else {
if (xrows - thisk > 0)
memmove((char *)DATAPTR(tmp),
(char *)DATAPTR(elem)+(thisk*size),
(xrows-thisk)*size);
for (m=xrows-thisk; m<xrows; m++)
for (int m=xrows-thisk; m<xrows; m++)
REAL(tmp)[m] = REAL(thisfill)[0];
}
copyMostAttrib(elem, tmp);
Expand All @@ -104,23 +104,23 @@ SEXP shift(SEXP obj, SEXP k, SEXP fill, SEXP type) {

case LGLSXP :
thisfill = PROTECT(coerceVector(fill, LGLSXP)); protecti++;
for (j=0; j<nk; j++) {
thisk = (INTEGER(k)[j] >= 0) ? INTEGER(k)[j] : -INTEGER(k)[j];
for (int j=0; j<nk; j++) {
int thisk = (kd[j] >= 0) ? kd[j] : -kd[j];
thisk = (xrows >= thisk) ? thisk : xrows;
SET_VECTOR_ELT(ans, i*nk+j, tmp=allocVector(LGLSXP, xrows) );
if ((stype == LAG && INTEGER(k)[j] >= 0) || (stype == LEAD && INTEGER(k)[j] < 0)) {
if ((stype == LAG && kd[j] >= 0) || (stype == LEAD && kd[j] < 0)) {
if (xrows - thisk > 0)
memmove((char *)DATAPTR(tmp)+(thisk*size),
(char *)DATAPTR(elem),
(xrows-thisk)*size);
for (m=0; m<thisk; m++)
for (int m=0; m<thisk; m++)
LOGICAL(tmp)[m] = LOGICAL(thisfill)[0];
} else {
if (xrows - thisk > 0)
memmove((char *)DATAPTR(tmp),
(char *)DATAPTR(elem)+(thisk*size),
(xrows-thisk)*size);
for (m=xrows-thisk; m<xrows; m++)
for (int m=xrows-thisk; m<xrows; m++)
LOGICAL(tmp)[m] = LOGICAL(thisfill)[0];
}
copyMostAttrib(elem, tmp);
Expand All @@ -129,14 +129,14 @@ SEXP shift(SEXP obj, SEXP k, SEXP fill, SEXP type) {

case STRSXP :
thisfill = PROTECT(coerceVector(fill, STRSXP)); protecti++;
for (j=0; j<nk; j++) {
for (int j=0; j<nk; j++) {
SET_VECTOR_ELT(ans, i*nk+j, tmp=allocVector(STRSXP, xrows) );
thisk = (INTEGER(k)[j] >= 0) ? INTEGER(k)[j] : -INTEGER(k)[j];
if ((stype == LAG && INTEGER(k)[j] >= 0) || (stype == LEAD && INTEGER(k)[j] < 0)) {
for (m=0; m<xrows; m++)
int thisk = (kd[j] >= 0) ? kd[j] : -kd[j];
if ((stype == LAG && kd[j] >= 0) || (stype == LEAD && kd[j] < 0)) {
for (int m=0; m<xrows; m++)
SET_STRING_ELT(tmp, m, (m < thisk) ? STRING_ELT(thisfill, 0) : STRING_ELT(elem, m - thisk));
} else {
for (m=0; m<xrows; m++)
for (int m=0; m<xrows; m++)
SET_STRING_ELT(tmp, m, (xrows-m <= thisk) ? STRING_ELT(thisfill, 0) : STRING_ELT(elem, m + thisk));
}
copyMostAttrib(elem, tmp);
Expand All @@ -146,14 +146,14 @@ SEXP shift(SEXP obj, SEXP k, SEXP fill, SEXP type) {

case VECSXP :
thisfill = PROTECT(coerceVector(fill, VECSXP)); protecti++;
for (j=0; j<nk; j++) {
for (int j=0; j<nk; j++) {
SET_VECTOR_ELT(ans, i*nk+j, tmp=allocVector(VECSXP, xrows) );
thisk = (INTEGER(k)[j] >= 0) ? INTEGER(k)[j] : -INTEGER(k)[j];
if ((stype == LAG && INTEGER(k)[j] >= 0) || (stype == LEAD && INTEGER(k)[j] < 0)) {
for (m=0; m<xrows; m++)
int thisk = (kd[j] >= 0) ? kd[j] : -kd[j];
if ((stype == LAG && kd[j] >= 0) || (stype == LEAD && kd[j] < 0)) {
for (int m=0; m<xrows; m++)
SET_VECTOR_ELT(tmp, m, (m < thisk) ? VECTOR_ELT(thisfill, 0) : VECTOR_ELT(elem, m - thisk));
} else {
for (m=0; m<xrows; m++)
for (int m=0; m<xrows; m++)
SET_VECTOR_ELT(tmp, m, (xrows-m <= thisk) ? VECTOR_ELT(thisfill, 0) : VECTOR_ELT(elem, m + thisk));
}
copyMostAttrib(elem, tmp);
Expand Down