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

allow :col => AsTable and :col => cols #2780

Merged
merged 2 commits into from
Jun 9, 2021
Merged

allow :col => AsTable and :col => cols #2780

merged 2 commits into from
Jun 9, 2021

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Jun 5, 2021

Fixes #2779

This is mildly breaking but I would classify the current behavior as a bug (oversight).

@bkamins bkamins added this to the 1.x milestone Jun 5, 2021
@bkamins bkamins requested a review from pdeffebach June 5, 2021 22:12
NEWS.md Outdated Show resolved Hide resolved
@@ -287,6 +293,11 @@ function normalize_selection(idx::AbstractIndex,
@nospecialize(sel::Pair{<:ColumnIndex, <:Base.Callable}), renamecols::Bool)
c = idx[first(sel)]
fun = last(sel)

if fun === AsTable
return normalize_selection(idx, first(sel) => identity => AsTable, renamecols)
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good. No reason to worry about copies because it's only a vector of named tuples, so we always allocate.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes - AsTable as destination will always allocate:

julia> df = DataFrame(a=1)
1×1 DataFrame
 Row │ a
     │ Int64
─────┼───────
   1 │     1

julia> df2 = select(df, :a => identity => AsTable, copycols=false)
1×1 DataFrame
 Row │ x1
     │ Int64
─────┼───────
   1 │     1

julia> df2.x1 === df.a
false

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.

Looks good, but given that it introduces a new error I wouldn't backport this to a patch release.

@bkamins
Copy link
Member Author

bkamins commented Jun 8, 2021

I want it to be added in 1.2 release.

@bkamins bkamins merged commit 886ebad into main Jun 9, 2021
@bkamins bkamins deleted the bk/split_column branch June 9, 2021 06:22
@bkamins
Copy link
Member Author

bkamins commented Jun 9, 2021

Thank you!

@pdeffebach
Copy link
Contributor

Can we get a release and a tag for this? DataFramesMeta has some weird results without it.

@bkamins
Copy link
Member Author

bkamins commented Jun 23, 2021

I would like to make a release, but here is the list of things that need to happen before: #2797 (comment)

Sorry for the delay.

@pdeffebach
Copy link
Contributor

Sounds good. Sorry for not looking at the sub-dataframe issue. I promise to look at it soon.

@bkamins
Copy link
Member Author

bkamins commented Jun 23, 2021

Sorry for not looking at the sub-dataframe issue. I promise to look at it soon.

No rush with this. But could you review the blocking issues please? Thank you!

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

Successfully merging this pull request may close these issues.

transform(df, :x => AsTable)` should probably work
3 participants