feat(dataframe): improve DataFrame::from_columns input types (#24630) - #24633
Conversation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24633 +/- ##
==========================================
- Coverage 81.60% 81.60% -0.01%
==========================================
Files 1123 1123
Lines 408898 408898
Branches 408898 408898
==========================================
- Hits 333670 333663 -7
- Misses 55625 55632 +7
Partials 19603 19603 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. I like the direction of making DataFrame::from_columns more flexible, but I found one compatibility issue with the public API change that I think needs to be addressed before merging.
I also left a small test coverage suggestion in case the broader AsRef<str> API is kept.
| .collect::<Vec<_>>(); | ||
|
|
||
| let arrays = columns | ||
| pub fn from_columns<I, S>(columns: I) -> Result<Self> |
There was a problem hiding this comment.
I think this public signature change is source-incompatible, even though the PR description currently calls it non-breaking.
Previously, DataFrame::from_columns(vec![]) inferred the argument as Vec<(&str, ArrayRef)>. With separate I and S generic parameters, S can no longer be inferred for an empty vec![], so the same call now fails with E0283. Cargo semver checks also report that the method now takes a different number of generic type parameters.
The PR description could also be clarified here. Vec was already supported. Arrays are the new addition, and this implementation goes further by accepting any compatible IntoIterator plus string-like column names.
Could we preserve the existing API compatibility while adding array support? For example, impl IntoIterator<Item = (&str, ArrayRef)> would support both arrays and the existing Vec use cases without introducing the extra type inference issue.
If supporting arbitrary string-like names is intentional, then I think this should instead be treated as an API change under the API-health policy, including adding api-change, updating the PR description, and providing upgrade guidance where appropriate.
There was a problem hiding this comment.
Thanks for pointing this out. I agree, this makes sense.
I don't want this PR to introduce any breaking changes. The goal is to extend the existing API to also support arrays while keeping the current Vec<(&str, ArrayRef)> usage fully compatible.
I'll rework the implementation to preserve the existing API and avoid the type inference issue.
|
|
||
| assert_eq!(df.schema().fields().len(), 13); | ||
| assert_eq!(df.clone().count().await?, 3); | ||
| let df1 = DataFrame::from_columns(columns.clone())?; |
There was a problem hiding this comment.
If the final API keeps S: AsRef<str> as an intentional feature, could we add a small test using String column names and a non-collection iterator, such as .into_iter().map(...)?
The current tests cover arrays and Vecs with &str names, but they do not compile-cover the broader string-like name and iterator contract.
There was a problem hiding this comment.
I agree, it makes sense to extend the tests to cover the IntoIterator API. Since I'm keeping the existing (&str, ArrayRef) item type for compatibility, I'll add a test with a non-collection iterator using .into_iter().map(...).
|
@kosiew Thanks for the valuable review! Your comments were clear and easy to understand. I've updated the API to preserve backward compatibility and added the requested test coverage. Could you please take another look when you have a chance? |
kosiew
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. The empty vec![] inference issue is fixed now, the names are narrowed back to &str as discussed, and the iterator test covers the intended IntoIterator behavior. I also don't see any additional follow-up defects.
There is still one API compatibility issue that needs to be resolved before this can be approved.
| .collect::<Vec<_>>(); | ||
|
|
||
| let arrays = columns | ||
| pub fn from_columns<'a, I>(columns: I) -> Result<Self> |
There was a problem hiding this comment.
Thanks for updating this. One API compatibility concern still remains here.
This changes the released non-generic from_columns(Vec<(&str, ArrayRef)>) signature to from_columns<'a, I>, so the SemVer/API-health issue is still present. cargo-semver-checks will report method_requires_different_generic_type_params, and downstream code that uses this method as a non-generic function item can break.
Using parameter-position impl IntoIterator<Item = (&str, ArrayRef)> may avoid that specific cargo-semver-checks diagnostic and would preserve normal Vec call syntax, but it is still an implicit generic parameter, so it would not be a strict compatibility fix either.
To preserve the existing public API, I think the safest option is to keep from_columns(Vec<...>) and add the iterator or array-taking behavior under a new method name. The old API could then be deprecated later according to policy if desired.
If this signature change is intentional instead, it should be treated and documented as a breaking API change under the API-health policy, including the api-change label and upgrade guidance.
There was a problem hiding this comment.
@kosiew Thanks for the clarification. I understand the remaining compatibility concern now.
My goal is to allow from_columns to accept arrays and other IntoIterator inputs while keeping the existing Vec usage working. I’d prefer to keep this behavior under from_columns rather than introduce a second method.
I agree that this is a breaking change under the API-health policy. I’ll update the PR accordingly, including adding the api-change label, updating the description, and providing upgrade guidance where appropriate.
Once the changes are ready, I’ll let you know and ask for another review.
cd5e470 to
115d90c
Compare
|
@kosiew thanks for the clarification. I've updated the PR: added upgrade guidance and updated the description about breaking changes. One question - could you help with adding And I think the PR is ready for review. Could you please take another look? |
|
can you resolve the merge conflicts? |
There was a problem hiding this comment.
Thanks for addressing the earlier feedback. I took another look at the follow-up changes and everything looks good to me, aside from the merge conflict.
The empty vec![] inference issue is fixed and covered by test_dataframe_from_columns_empty. The API is now appropriately limited to &str column names, so the earlier AsRef<str> suggestion no longer applies. There is also coverage for a non-collection iterator in test_dataframe_from_columns_with_iterator.
The remaining function-item signature incompatibility is now clearly treated as an intentional breaking change for the major release. The PR description documents the break, migration guidance has been added to the 56.0.0 upgrade guide, the API-change label is present, and the SemVer CI check passes.
08005d4 to
1a5e7c2
Compare
|
@kosiew Thanks for the feedback! I fixed the merge conflict and updated |
|
🚀 |
Which issue does this PR close?
Rationale for this change
Improve the
DataFrame::from_columnsAPI and give users more flexibility when constructing aDataFramefrom columns.What changes are included in this PR?
DataFrame::from_columnsto acceptIntoIteratorof columns.arraysandVecsof columns.Are these changes tested?
Yes. Tests cover both
arrayandVecinputs and verify the resulting schema, data types, row count, and values.Are there any user-facing changes?
Yes. This is a breaking API change.
DataFrame::from_columnsnow accepts anIntoIteratorof columns instead of specifically accepting aVec. ExistingVecusage continues to work, while users can also passarraysand other compatible iterators.Users relying on the exact non-generic function signature may need to update their code to account for the new generic API.