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

Column insertion in DataFrameRow #3391

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Column insertion in DataFrameRow #3391

wants to merge 5 commits into from

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Oct 18, 2023

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.

Copy link
Member

@nalimilan nalimilan left a 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)))
Copy link
Member

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?

Copy link
Member Author

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.:

Do you think we should do a systematic review of them and fix this everywhere?

Copy link
Member

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.

src/dataframerow/dataframerow.jl Outdated Show resolved Hide resolved
src/dataframerow/dataframerow.jl Outdated Show resolved Hide resolved
@bkamins
Copy link
Member Author

bkamins commented Oct 22, 2023

we could allow deletecols!

Now I just added functions that match what we support for SubDataFrame. We could support e.g. deletecols! both for SubDataFrame and DataFrameRow, but I would wait till someone asks for it. Column adding is useful as it fills omitted rows with missing and this is what this PR does.

@nalimilan
Copy link
Member

Now I just added functions that match what we support for SubDataFrame. We could support e.g. deletecols! both for SubDataFrame and DataFrameRow, but I would wait till someone asks for it. Column adding is useful as it fills omitted rows with missing and this is what this PR does.

Right, I just wanted to think about the broader implications of this PR. Mirroring what we do for SubDataFrame sounds like a good rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow adding columns to DataFrameRow just like to SubDataFrame
2 participants