Skip to content

Add row names to null data.table on assign#4609

Merged
mattdowle merged 17 commits intomasterfrom
null_dt_rows_4597
Apr 29, 2021
Merged

Add row names to null data.table on assign#4609
mattdowle merged 17 commits intomasterfrom
null_dt_rows_4597

Conversation

@ColeMiller1
Copy link
Copy Markdown
Contributor

@ColeMiller1 ColeMiller1 commented Jul 17, 2020

Closes #4597.

I will do the news release after 1.13.0 is released. Also, will update test tomorrow to make this pass.

library(data.table)

dt = data.table()
dt[, new := 1:5]
attr(dt, "row.names") ##fixed!
#> [1] 1 2 3 4 5

## warning for now - easy to change to error with feedback. 
## data.table::groupingsets depended on this so there may be other issues with a hard change
dt = data.table(x = integer())
dt[, new := 1:5]

## Warning message:
## In `[.data.table`(dt, , `:=`(new, 1:5)) :
##   The data.table has 0 rows but there are 5 rows being assigned. The value will be ignored but new column(s) will be 
##   added. This warning will be upgraded to error in next release
dt
#> Empty data.table (0 rows and 2 cols): x,new

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 17, 2020

Codecov Report

Merging #4609 (7a5263b) into master (b284e37) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4609   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files          73       73           
  Lines       14450    14455    +5     
=======================================
+ Hits        14367    14372    +5     
  Misses         83       83           
Impacted Files Coverage Δ
src/assign.c 99.70% <100.00%> (+<0.01%) ⬆️

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 b284e37...7a5263b. Read the comment docs.

@ColeMiller1 ColeMiller1 added this to the 1.13.7 milestone Feb 3, 2021
@ColeMiller1
Copy link
Copy Markdown
Contributor Author

@jangorecki let me know if I should split this up. The updating of row.names of data.table()[, new_var := 1:5] makes sense. However, I'm uncertain about data.table(x = integer())[, new_var := 1]. Current behavior is a zero-length variable being assigned. Proposed behavior is a warning with long-term aim of being an error. I did it as a single PR because they were relatively closely related.

@mattdowle
Copy link
Copy Markdown
Member

Great fix, thanks.
As you alluded to above, and influenced by recent #4262, I went ahead and removed the new warning and reverted the changed tests. I'm (now) viewing data.table(x = integer())[, new_var := 1] as recycling of the length-1 to match the length-0 data. Happily, this is already what the tests tested, so my change to this PR brings that side of it back to current behavior so it can be merged to fix the row.names bug. Can always reconsider, discuss, and have another PR in future as you said.

@mattdowle mattdowle merged commit 0f94212 into master Apr 29, 2021
@mattdowle mattdowle deleted the null_dt_rows_4597 branch April 29, 2021 21:07
@jangorecki jangorecki modified the milestones: 1.14.9, 1.15.0 Oct 29, 2023
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.

data.table not compatible with ggplot when it was generated from empty data.table

3 participants