Skip to content

get reordering of columns on reference#4212

Merged
mattdowle merged 8 commits intoRdatatable:masterfrom
ColeMiller1:get_col_order
Feb 17, 2020
Merged

get reordering of columns on reference#4212
mattdowle merged 8 commits intoRdatatable:masterfrom
ColeMiller1:get_col_order

Conversation

@ColeMiller1
Copy link
Copy Markdown
Contributor

@ColeMiller1 ColeMiller1 commented Jan 30, 2020

Closes #4089.

Current behavior will re-order columns when using (m)get even when .SDcols is present. This can be dangerous when updating in place:

library(data.table)
dt <- data.table(x = 1, y = 2, z = 3)
cols <- c("y","x")

dt[, (cols) := lapply(.SD[get("x") == 1],function(x){x}), .SDcols = cols ,by = z][]
       x     y     z
   <num> <num> <num>
1:     2     1     3

Proposed will not re-order the columns when .SDcols argument is present.

dt[, (cols) := lapply(.SD[get("x") == 1],function(x){x}), .SDcols = cols ,by = z][]
       x     y     z
   <num> <num> <num>
1:     1     2     3

Also, some code was re-arranged to address a TODO comment related to this section of [.data.table.

This also includes slight refactoring to eliminate one *TODO* comment
@ColeMiller1 ColeMiller1 marked this pull request as ready for review January 30, 2020 04:55
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 30, 2020

Codecov Report

Merging #4212 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4212      +/-   ##
==========================================
+ Coverage   99.61%   99.61%   +<.01%     
==========================================
  Files          72       72              
  Lines       13874    13904      +30     
==========================================
+ Hits        13820    13850      +30     
  Misses         54       54
Impacted Files Coverage Δ
R/merge.R 100% <ø> (ø) ⬆️
src/subset.c 100% <ø> (ø) ⬆️
R/as.data.table.R 100% <ø> (ø) ⬆️
src/assign.c 100% <ø> (ø) ⬆️
R/xts.R 100% <100%> (ø) ⬆️
src/fifelse.c 100% <100%> (ø) ⬆️
src/fwriteR.c 100% <100%> (ø) ⬆️
src/chmatch.c 100% <100%> (ø) ⬆️
src/dogroups.c 100% <100%> (ø) ⬆️
R/setkey.R 100% <100%> (ø) ⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d11fe0...bb5adf5. Read the comment docs.

Copy link
Copy Markdown
Member

@jangorecki jangorecki left a comment

Choose a reason for hiding this comment

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

Very nice!
Could we make this warning avoidable? see #3848

Comment thread R/data.table.R Outdated
Comment thread R/data.table.R Outdated
Comment thread R/data.table.R Outdated
Comment thread R/data.table.R Outdated
Comment thread R/data.table.R Outdated
@ColeMiller1
Copy link
Copy Markdown
Contributor Author

ColeMiller1 commented Feb 2, 2020

Thank you both for the great comments. Main changes

  1. Removed the warning
  2. Updated to cat(gettextf("New ansvar ...", brackify(ansvars), domain = "R-data.table")
  3. Re-organized the comments and cat statements to better match the order of what I deleted

Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico left a comment

Choose a reason for hiding this comment

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

Looks great!

Can you add a test of the new behavior?

@ColeMiller1 ColeMiller1 mentioned this pull request Feb 2, 2020
4 tasks
@mattdowle mattdowle added this to the 1.12.9 milestone Feb 17, 2020
@mattdowle
Copy link
Copy Markdown
Member

Quite an involved first time contribution! That part of the code is hard work, thank you. I've invited you to be project member; please accept invitation that shows in your github profile or projects tab. I'll add you to contributor list in a follow up commit.

@mattdowle mattdowle merged commit 07931a2 into Rdatatable:master Feb 17, 2020
mattdowle added a commit that referenced this pull request Feb 17, 2020
… this block (%iscall% and a github ref number update); planned follow up to #4212
@jangorecki jangorecki modified the milestones: 1.12.11, 1.12.9 May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

lapply on .SD reorder columns when using get() in .SD

4 participants