kosiew commented on code in PR #24633:
URL: https://github.com/apache/datafusion/pull/24633#discussion_r3879171425
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -6982,10 +6982,10 @@ async fn test_dataframe_from_columns() -> Result<()> {
("f32", f32s),
("f64", f64s),
("str", strings),
- ])?;
+ ];
- assert_eq!(df.schema().fields().len(), 13);
- assert_eq!(df.clone().count().await?, 3);
+ let df1 = DataFrame::from_columns(columns.clone())?;
Review 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 `Vec`s with `&str` names, but they do not
compile-cover the broader string-like name and iterator contract.
##########
datafusion/core/src/dataframe/mod.rs:
##########
@@ -2628,17 +2628,20 @@ impl DataFrame {
/// # Ok(())
/// # }
/// ```
- pub fn from_columns(columns: Vec<(&str, ArrayRef)>) -> Result<Self> {
- let fields = columns
- .iter()
- .map(|(name, array)| Field::new(*name, array.data_type().clone(),
true))
- .collect::<Vec<_>>();
-
- let arrays = columns
+ pub fn from_columns<I, S>(columns: I) -> Result<Self>
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
