Copilot commented on code in PR #22904:
URL: https://github.com/apache/datafusion/pull/22904#discussion_r3394641216
##########
datafusion/core/src/dataframe/mod.rs:
##########
@@ -2458,19 +2458,14 @@ impl DataFrame {
/// .read_csv("tests/data/example.csv", CsvReadOptions::new())
/// .await?;
/// // Fill nulls in only columns "a" and "c":
- /// let df = df.fill_null(ScalarValue::from(0), vec!["a".to_owned(),
"c".to_owned()])?;
+ /// let df = df.fill_null(&ScalarValue::from(0), &["a", "c"])?;
/// // Fill nulls across all columns:
- /// let df = df.fill_null(ScalarValue::from(0), vec![])?;
+ /// let df = df.fill_null(&ScalarValue::from(0), &[])?;
/// # Ok(())
/// # }
/// ```
- #[expect(clippy::needless_pass_by_value)]
- pub fn fill_null(
- &self,
- value: ScalarValue,
- columns: Vec<String>,
- ) -> Result<DataFrame> {
- self.fill_columns(&value, &columns, &coalesce(), |_| true)
+ pub fn fill_null(&self, value: &ScalarValue, columns: &[&str]) ->
Result<DataFrame> {
+ self.fill_columns(value, columns, &coalesce(), |_| true)
Review Comment:
`columns: &[&str]` is quite restrictive for callers who already have
`Vec<String>` / `&[String]` column names—this forces an extra conversion step
to produce a `Vec<&str>`. To keep the “no allocation” goal while improving
ergonomics, consider making `fill_null` generic over `AsRef<str>` (e.g.,
`fill_null<S: AsRef<str>>(&self, value: &ScalarValue, columns: &[S])`) so
callers can pass either `&["a", "c"]` or `&vec_of_strings` directly.
##########
docs/source/library-user-guide/upgrading/55.0.0.md:
##########
@@ -25,6 +25,48 @@
in this section pertains to features and changes that have already been merged
to the main branch and are awaiting release in this version.
+### `DataFrame::fill_null` now borrows its arguments
+
+`DataFrame::fill_null` previously took its arguments by value:
+
+```rust,ignore
+// Before
+pub fn fill_null(
+ &self,
+ value: ScalarValue,
+ columns: Vec<String>,
+) -> Result<DataFrame>
+```
+
+It now borrows them, matching the signature of the newly added
+`DataFrame::fill_nan`:
+
+```rust,ignore
+// After
+pub fn fill_null(
+ &self,
+ value: &ScalarValue,
+ columns: &[&str],
+) -> Result<DataFrame>
+```
+
+This lets callers pass a borrowed `ScalarValue` and slice literals (or
+`&str` column names) without first allocating owned `String`s.
+
+**Migration guide:**
+
+Borrow the value and pass a slice of `&str` instead of an owned `Vec<String>`:
+
+```rust,ignore
+// Before
+let df = df.fill_null(ScalarValue::from(0), vec!["a".to_owned(),
"c".to_owned()])?;
+let df = df.fill_null(ScalarValue::from(0), vec![])?;
+
+// After
+let df = df.fill_null(&ScalarValue::from(0), &["a", "c"])?;
+let df = df.fill_null(&ScalarValue::from(0), &[])?;
Review Comment:
The migration guide says callers can “pass a borrowed `ScalarValue`”, but
the “After” examples still take a reference to a temporary
(`&ScalarValue::from(0)`), which doesn’t demonstrate the intended pattern
(reusing an existing `ScalarValue`). Consider updating the example to bind the
scalar to a variable first (e.g., `let value = ScalarValue::from(0);
df.fill_null(&value, ...)`) and optionally add a brief example for dynamic
column names (e.g., converting `Vec<String>` to `Vec<&str>`).
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -6472,11 +6472,8 @@ async fn test_fill_null() -> Result<()> {
// Use fill_null to replace nulls on each column.
let df_filled = df
- .fill_null(ScalarValue::Int32(Some(0)), vec!["a".to_string()])?
- .fill_null(
- ScalarValue::Utf8(Some("default".to_string())),
- vec!["b".to_string()],
- )?;
+ .fill_null(&ScalarValue::Int32(Some(0)), &["a"])?
+ .fill_null(&ScalarValue::Utf8(Some("default".to_string())), &["b"])?;
Review Comment:
These tests borrow temporaries (`&ScalarValue::...`) and allocate
`"default".to_string()` inline. To better reflect typical usage with borrowed
values (and make the test intent a bit clearer), consider binding the
`ScalarValue`(s) to local variables and borrowing those.
--
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]
