Skip to content

feat(dataframe): improve DataFrame::from_columns input types (#24630) - #24633

Merged
kosiew merged 6 commits into
apache:mainfrom
cj-zhukov:cj-zhukov/improve-dataFrame-from_columns-input-types
Sep 3, 2026
Merged

feat(dataframe): improve DataFrame::from_columns input types (#24630)#24633
kosiew merged 6 commits into
apache:mainfrom
cj-zhukov:cj-zhukov/improve-dataFrame-from_columns-input-types

Conversation

@cj-zhukov

@cj-zhukov cj-zhukov commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Improve the DataFrame::from_columns API and give users more flexibility when constructing a DataFrame from columns.

What changes are included in this PR?

  • Generalize DataFrame::from_columns to accept IntoIterator of columns.
  • This allows users to pass both arrays and Vecs of columns.
  • Update tests to cover both input forms.

Are these changes tested?

Yes. Tests cover both array and Vec inputs 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_columns now accepts an IntoIterator of columns instead of specifically accepting a Vec. Existing Vec usage continues to work, while users can also pass arrays and 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.

@github-actions github-actions Bot added the core Core DataFusion crate label Aug 24, 2026
@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown

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
     Cloning apache/main
    Building datafusion v55.0.0 (current)
       Built [  61.700s] (current)
     Parsing datafusion v55.0.0 (current)
      Parsed [   0.035s] (current)
    Building datafusion v55.0.0 (baseline)
       Built [  59.689s] (baseline)
     Parsing datafusion v55.0.0 (baseline)
      Parsed [   0.035s] (baseline)
    Checking datafusion v55.0.0 -> v55.0.0 (no change; assume patch)
     Checked [   0.629s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip

--- failure method_requires_different_generic_type_params: method now requires a different number of generic type parameters ---

Description:
A method now requires a different number of generic type parameters than it used to. Uses of this method that supplied the previous number of generic types will be broken.
        ref: https://doc.rust-lang.org/reference/items/generics.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/method_requires_different_generic_type_params.ron

Failed in:
  datafusion::prelude::dataframe::DataFrame::from_columns takes 1 generic types instead of 0, in /home/runner/work/datafusion/datafusion/datafusion/core/src/dataframe/mod.rs:2631
  datafusion::dataframe::DataFrame::from_columns takes 1 generic types instead of 0, in /home/runner/work/datafusion/datafusion/datafusion/core/src/dataframe/mod.rs:2631
  datafusion::prelude::DataFrame::from_columns takes 1 generic types instead of 0, in /home/runner/work/datafusion/datafusion/datafusion/core/src/dataframe/mod.rs:2631

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [ 123.979s] datafusion

@github-actions github-actions Bot added the auto detected api change Auto detected API change label Aug 24, 2026
@codecov-commenter

codecov-commenter commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.60%. Comparing base (da89c7c) to head (1a5e7c2).
⚠️ Report is 1 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@cj-zhukov,

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.

Comment thread datafusion/core/src/dataframe/mod.rs Outdated
.collect::<Vec<_>>();

let arrays = columns
pub fn from_columns<I, S>(columns: I) -> Result<Self>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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())?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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(...).

@cj-zhukov

Copy link
Copy Markdown
Contributor Author

@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 kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@cj-zhukov,

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>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Aug 30, 2026
@cj-zhukov
cj-zhukov force-pushed the cj-zhukov/improve-dataFrame-from_columns-input-types branch from cd5e470 to 115d90c Compare August 30, 2026 18:42
@cj-zhukov

Copy link
Copy Markdown
Contributor Author

@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 api-change to the PR because I couldn't do it by myself.

And I think the PR is ready for review. Could you please take another look?

@kosiew

kosiew commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

@cj-zhukov

can you resolve the merge conflicts?

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@cj-zhukov,

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.

@cj-zhukov
cj-zhukov force-pushed the cj-zhukov/improve-dataFrame-from_columns-input-types branch from 08005d4 to 1a5e7c2 Compare September 1, 2026 18:38
@cj-zhukov

Copy link
Copy Markdown
Contributor Author

@kosiew Thanks for the feedback!

I fixed the merge conflict and updated test_dataframe_from_columns_empty to be synchronous, as it doesn't perform any async operations.

@kosiew
kosiew added this pull request to the merge queue Sep 3, 2026
@kosiew

kosiew commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

🚀
@cj-zhukov
Thank you for your contribution.

Merged via the queue into apache:main with commit e36c5f8 Sep 3, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto detected api change Auto detected API change core Core DataFusion crate documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants