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.

Sign up for GitHub

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

Jump to bottom

Fix default copycols for Tables.jl constructor #2710

Merged
merged 2 commits into from
Apr 12, 2021
Merged

Fix default copycols for Tables.jl constructor #2710

merged 2 commits into from
Apr 12, 2021

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Apr 11, 2021

No description provided.

@bkamins
Copy link
Member

bkamins commented Apr 11, 2021

Looks good. Can you add a test please? Thank you!

@bkamins bkamins added the ecosystem Issues in DataFrames.jl ecosystem label Apr 11, 2021
@bkamins bkamins added this to the 1.0 milestone Apr 11, 2021
@bkamins bkamins requested a review from nalimilan April 11, 2021 16:40
@bkamins
Copy link
Member

bkamins commented Apr 12, 2021

@quinnj - apart from adding tests for new functionality also old tests need updating as we tested against the old behavior.

@quinnj
Copy link
Member Author

quinnj commented Apr 12, 2021

Alright, fixed an issue w/ dispatching to fromcolumns for CopiedColumns and added an additional test that actually uses the internal CopiedColumns code path; we had a test when CopiedColumns was passed directly to DataFrame constructor, but, as is the case for Arrow.Table and CSV.File, they return CopiedColumns for the result of Tables.columns. I think this is in good shape now and will merge once CI passes.

@quinnj quinnj merged commit 049e32c into main Apr 12, 2021
@quinnj quinnj deleted the quinnj-patch-1 branch April 12, 2021 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkamins bkamins bkamins approved these changes

@nalimilan nalimilan Awaiting requested review from nalimilan

Assignees
No one assigned
Labels
ecosystem Issues in DataFrames.jl ecosystem
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@quinnj @bkamins