Skip to content
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.

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

prevent setDT on data.frame with matrix columns #3770

Merged
merged 7 commits into from
Aug 28, 2019
Merged

Conversation

MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Aug 16, 2019

Closes #3760
Closes #2936

Not sure this is an ideal solution, but it works & indeed as.data.table is the appropriate function for such objects.


Also realizing this catches #2936 from getting to the point where it would have segfaulted (doesn't segfault anymore)

Copy link
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.

looks fine, only address tests failure

@MichaelChirico
Copy link
Member Author

@jangorecki can you think of any other types of validations we'd want to do upfront here? if so maybe best to have a validate_dt function we can build up starting from here.

@MichaelChirico
Copy link
Member Author

looking for NULL dim failed test 1980, which has an array with length(dim(x)) == 1L that ends up being setkey'd (confirmed that in this case, setDT wipes out the dim). Played around with changing as.data.table.list to route this case through as.data.table but we'd have to route through as.data.table.array, which fails because there's a check that length(dim(x)) >= 3 there.

If we change this to route this case to as.data.table.matrix instead of erroring, the solution I had wound up with the wrong name:

y = as.array(1:5)
names(data.table(y))
# [1] 'x'

So I gave up on that route; the one here does fine. Just recording my thoughts for posterity.

@codecov
Copy link

codecov bot commented Aug 16, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3770      +/-   ##
==========================================
+ Coverage   99.41%   99.41%   +<.01%     
==========================================
  Files          71       71              
  Lines       13222    13225       +3     
==========================================
+ Hits        13145    13148       +3     
  Misses         77       77
Impacted Files Coverage Δ
R/data.table.R 100% <100%> (ø) ⬆️

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 d75265f...94b0cc6. Read the comment docs.

@MichaelChirico MichaelChirico changed the title Closes #3760 -- prevent setDT for data.frame w matrix columns Closes #3760 and #2936 -- prevent setDT for data.frame w matrix columns Aug 24, 2019
@mattdowle mattdowle changed the title Closes #3760 and #2936 -- prevent setDT for data.frame w matrix columns prevent setDT on data.frame with matrix columns Aug 28, 2019
@mattdowle mattdowle added this to the 1.12.4 milestone Aug 28, 2019
@mattdowle mattdowle merged commit 6a7246e into master Aug 28, 2019
@mattdowle mattdowle deleted the setdt_dim_col branch August 28, 2019 01:13
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.

setDT fails to detect invalid columns -> obscure error downstream segfault unlisting a nested data.frame
3 participants