Skip to content

gshift() of one column returns a vector, not list(vector) #5950

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
MichaelChirico merged 2 commits into from
Feb 25, 2024
Merged

gshift() of one column returns a vector, not list(vector) #5950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9e3b0be
gshift() of one column returns a vector, not list(vector)
MichaelChirico Feb 22, 2024
a4889f0
Merge branch 'master' into gshift-1
MichaelChirico Feb 25, 2024
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
5 changes: 4 additions & 1 deletion inst/tests/tests.Rraw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17829,9 +17829,12 @@ for (col in names(DT)[-1]) {
}
a = 1:2 # fill argument with length > 1 which is not a call
test(2224.2, DT[, shift(i, fill=a), by=x], error="fill must be a vector of length 1")
DT = data.table(x=pairlist(1), g=1)
# unsupported type as argument
DT = data.table(x=pairlist(1), g=1)
test(2224.3, DT[, shift(x), g], error="Type 'list' is not supported by GForce gshift.")
# single vector is un-list()'d, consistently with plain shift(), #5939
DT = data.table(x=1, g=1)
test(2224.4, DT[, .(shift(x)), g], DT[, shift(x), g])

# groupingsets by named by argument
test(2225.1, groupingsets(data.table(iris), j=sum(Sepal.Length), by=c('Sp'='Species'), sets=list('Species')),
Expand Down
3 changes: 2 additions & 1 deletion src/gsumm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1268,6 +1268,7 @@ SEXP gshift(SEXP x, SEXP nArg, SEXP fillArg, SEXP typeArg) {
copyMostAttrib(x, tmp); // needed for integer64 because without not the correct class of int64 is assigned
}
UNPROTECT(nprotect);
return(ans);
// consistency with plain shift(): "strip" the list in the 1-input case, for convenience
return isVectorAtomic(x) && length(ans) == 1 ? VECTOR_ELT(ans, 0) : ans;
Copy link
Member Author

@MichaelChirico MichaelChirico Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ben-schwen do we actually need the isVectorAtomic() check in the gshift case? Is it possible to get a list() here in gshift?

Copy link
Member

@ben-schwen ben-schwen Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now no, the only supported types are the six atomic vector types (otherwise we would error before in the macro switch). AFAIR I deactivated the support for list columns because we use coerceAs for the fill argument and coerceAs only supports atomic vectors.

Copy link
Member Author

@MichaelChirico MichaelChirico Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see also #4586

Copy link
Member Author

@MichaelChirico MichaelChirico Feb 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave the isVectorAtomic() check, simpler to keep consistency with "plain" shift.

}