-
Notifications
You must be signed in to change notification settings - Fork 370
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
Column insertion in DataFrameRow #3391
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Have you thought about other functions that we could want to implement (or not) for DataFrameRow
? For example, we could allow deletecols!
, but it could also be a bit more dangerous...
"columns of its parent data frame is disallowed")) | ||
end | ||
T = typeof(val) | ||
newcol = similar(val, Union{T,Missing}, nrow(parent(dfr))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Tables.allocate_column
? similar
is for arrays.
Or maybe just call insertcols!
to avoid duplication? Is that slower?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very good point. I used similar
, because we already had it in
https://github.com/JuliaData/DataFrames.jl/blob/main/src/subdataframe/subdataframe.jl#L207
which also requires fixing then.
Other places with the same issue are e.g.:
newcol = similar(old_col, promote_type(T, S), length(old_col)) DataFrames.jl/src/other/broadcasting.jl
Line 204 in 8c32d53
col = similar(Vector{typeof(v)}, nrow(df)) DataFrames.jl/src/dataframe/insertion.jl
Line 313 in 8c32d53
newcol = similar(df1_c, promote_type(S, T), targetrows)
Do you think we should do a systematic review of them and fix this everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we followed the policy that similar
is called when we have a vector to pass to it, and otherwise allocatecolumn
is used. Apparently that's not really the case, given the example in broadcasting.jl. The two other examples look correct to me. We could try changing the broadcasting.jl one and check whether other places have the same issue.
Now I just added functions that match what we support for |
Co-authored-by: Milan Bouchet-Valat <[email protected]>
…rames.jl into bk/row_insertion
Right, I just wanted to think about the broader implications of this PR. Mirroring what we do for |
Fixes #3390
@nalimilan - I just did the initial implementation. Let us first decide if we like the functionality and if yes, I will document and test it.