Jefffrey commented on code in PR #18415:
URL: https://github.com/apache/datafusion/pull/18415#discussion_r2562078948
##########
datafusion/common/src/datatype.rs:
##########
@@ -61,7 +62,54 @@ impl DataTypeExt for DataType {
}
/// DataFusion extension methods for Arrow [`Field`] and [`FieldRef`]
+///
+/// This trait is implemented for both [`Field`] and [`FieldRef`] and
+/// provides convenience methods for efficiently working with both types.
+///
+/// For [`FieldRef`], the methods will attempt to unwrap the `Arc`
+/// to avoid unnecessary cloning when possible.
pub trait FieldExt {
+ /// Ensure the field is named `new_name`, returning the given field if the
+ /// name matches, and a new field if not.
+ ///
+ /// This method avoids `clone`ing fields and names if the name is the same
+ /// as the field's existing name.
+ ///
+ /// Example:
+ /// ```
+ /// # use std::sync::Arc;
+ /// # use arrow::datatypes::{DataType, Field};
+ /// # use datafusion_common::datatype::FieldExt;
+ /// let int_field = Field::new("my_int", DataType::Int32, true);
+ /// // rename to "your_int"
+ /// let renamed_field = int_field.renamed("your_int");
+ /// assert_eq!(renamed_field.name(), "your_int");
+ /// ```
+ fn renamed(self, new_name: &str) -> Self;
Review Comment:
I wonder for these new methods if there is existing documentation elsewhere
we'd need to upgrade to make these new methods more visible? i.e. "use these
new methods if possible", though not sure how relevant might be considering
these mainly replace the old `as_ref from Arc -> clone -> modify field -> wrap
in new Arc` loop 🤔
##########
docs/source/library-user-guide/upgrading.md:
##########
@@ -25,6 +25,21 @@
You can see the current [status of the `52.0.0`release
here](https://github.com/apache/datafusion/issues/18566)
+### Changes to DFSchema API
+
+To permit more efficient planning, several methods on `DFSchema` have been
+changed to return references to the underlying [`&FieldRef`] rather than
+[`&Field`[. This allows planners to more cheaply copy the references via
Review Comment:
```suggestion
[`&Field`]. This allows planners to more cheaply copy the references via
```
##########
datafusion/common/src/dfschema.rs:
##########
@@ -353,13 +353,13 @@ impl DFSchema {
/// Returns an immutable reference of a specific `Field` instance selected
using an
Review Comment:
Perhaps need to update some of these method docstrings since they assume
returning a `Field`
--
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]
