alamb commented on code in PR #19549:
URL: https://github.com/apache/datafusion/pull/19549#discussion_r2688105570
##########
datafusion/functions/src/utils.rs:
##########
@@ -410,7 +410,7 @@ pub mod test {
#[test]
fn test_decimal32_to_i32() {
- let cases: [(i32, i8, Either<i32, String>); _] = [
+ let cases: [(i32, i8, Either<i32, String>); 10] = [
Review Comment:
why these changes?
##########
datafusion/core/src/dataframe/mod.rs:
##########
@@ -445,15 +445,31 @@ impl DataFrame {
/// # Ok(())
/// # }
/// ```
- pub fn drop_columns(self, columns: &[&str]) -> Result<DataFrame> {
+ pub fn drop_columns<T>(self, columns: &[T]) -> Result<DataFrame>
+ where
+ T: Into<Column> + Clone,
Review Comment:
I was wondering why the `Clone` bound is needed. In case anyone else is
curious -- it turns out it needed because what is passed in is `&[T]` (vs
something like `[T]`)
```
error[E0277]: the trait bound `datafusion_common::Column:
std::convert::From<&T>` is not satisfied
--> datafusion/core/src/dataframe/mod.rs:455:42
|
455 | let column: Column = col.into();
| ^^^^ the trait
`std::convert::From<&T>` is not implemented for `datafusion_common::Column`
|
= note: required for `&T` to implement
`std::convert::Into<datafusion_common::Column>`
help: consider extending the `where` clause, but there might be an
alternative better way to express this requirement
|
450 | T: Into<Column>, datafusion_common::Column:
std::convert::From<&T>
|
+++++++++++++++++++++++++++++++++++++++++++++++++
```
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -594,7 +696,7 @@ async fn drop_with_periods() -> Result<()> {
let ctx = SessionContext::new();
ctx.register_batch("t", batch)?;
- let df = ctx.table("t").await?.drop_columns(&["f.c1"])?;
+ let df = ctx.table("t").await?.drop_columns(&["\"f.c1\""])?;
Review Comment:
while this is technically a breaking API change, I think it is reasonable to
treat it as a bug fix
--
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]
