alamb commented on code in PR #9104:
URL: https://github.com/apache/arrow-datafusion/pull/9104#discussion_r1476751851
##########
datafusion/common/src/dfschema.rs:
##########
@@ -237,6 +301,7 @@ impl DFSchema {
}
}
self.fields.extend(fields_to_add);
+ self.fields_map = Self::get_fields_map(&self.fields);
Review Comment:
since merge is one of the hot paths, could we update self.fields_map rather
than recomputing it 🤔
##########
datafusion/common/src/dfschema.rs:
##########
@@ -112,6 +112,10 @@ pub struct DFSchema {
metadata: HashMap<String, String>,
/// Stores functional dependencies in the schema.
functional_dependencies: FunctionalDependencies,
+ /// Fields map
+ /// key - fully qualified field name
+ /// value - field index in schema
+ fields_map: BTreeMap<String, usize>,
Review Comment:
If you use a `String` here it still requires a lot of copying
What if you made something more specialized like:
```rust
fields_map: BTreeMap<(Option<OwnedTableReference>, Name), usize>
```
I think that would then let you look up elements in the map without having
to construct an owned name
##########
datafusion/common/src/dfschema.rs:
##########
@@ -865,11 +880,13 @@ impl DFField {
/// Returns a string to the `DFField`'s qualified name
pub fn qualified_name(&self) -> String {
- if let Some(qualifier) = &self.qualifier {
- format!("{}.{}", qualifier, self.field.name())
- } else {
- self.field.name().to_owned()
- }
+ Self::make_qualified_name(self.qualifier.as_ref(), self.name())
+ }
+
+ pub fn make_qualified_name(qualifier: Option<&TableReference>, name: &str)
-> String {
Review Comment:
I agree making it non pub would be good.
Also, the fact it returns an owned `String` seems like it still adds a lot
of copying. I left a suggestion above on how to maybe improve this
--
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]
