alamb commented on code in PR #9595:
URL: https://github.com/apache/arrow-datafusion/pull/9595#discussion_r1530796276
##########
datafusion/core/src/dataframe/mod.rs:
##########
@@ -1240,14 +1249,13 @@ impl DataFrame {
let mut col_exists = false;
let mut fields: Vec<Expr> = plan
.schema()
- .fields()
.iter()
- .map(|f| {
- if f.name() == name {
+ .map(|(qualifier, field)| {
+ if field.name() == name {
col_exists = true;
new_column.clone()
} else {
- col(f.qualified_column())
+ col(Column::new(qualifier.cloned(), field.name()))
Review Comment:
This pattern shows up enough maybe we could make it a from impl
Something like
And then a
```rust
impl From<(Option<&OwnedTableReference>, &Field)> for Column {
...
}
```
And then this code would look like
```rust
Column::from(qualifier, field)
```
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -374,28 +376,44 @@ impl ExprSchemable for Expr {
///
/// So for example, a projected expression `col(c1) + col(c2)` is
/// placed in an output field **named** col("c1 + c2")
- fn to_field(&self, input_schema: &dyn ExprSchema) -> Result<DFField> {
+ fn to_field(
+ &self,
+ input_schema: &dyn ExprSchema,
+ ) -> Result<(Option<OwnedTableReference>, Arc<Field>)> {
Review Comment:
same thing here, can we avoid copying the table reference? This
OwnedTableReference will force a string copy
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -374,28 +376,44 @@ impl ExprSchemable for Expr {
///
/// So for example, a projected expression `col(c1) + col(c2)` is
/// placed in an output field **named** col("c1 + c2")
- fn to_field(&self, input_schema: &dyn ExprSchema) -> Result<DFField> {
+ fn to_field(
+ &self,
+ input_schema: &dyn ExprSchema,
+ ) -> Result<(Option<OwnedTableReference>, Arc<Field>)> {
Review Comment:
Maybe we need some way to make the table reference `Arc<str>` instead of a
string 🤔 -- maybe that can be a follow on PR (rather than trying to use the
borrow checker)
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -46,7 +46,10 @@ pub trait ExprSchemable {
fn metadata(&self, schema: &dyn ExprSchema) -> Result<HashMap<String,
String>>;
/// convert to a field with respect to a schema
- fn to_field(&self, input_schema: &dyn ExprSchema) -> Result<DFField>;
+ fn to_field(
+ &self,
+ input_schema: &dyn ExprSchema,
+ ) -> Result<(Option<OwnedTableReference>, Arc<Field>)>;
Review Comment:
Does this need to be `Owned` ? Or can it be just a TableReference?
##########
datafusion/common/src/dfschema.rs:
##########
@@ -119,74 +120,113 @@ impl DFSchema {
/// Creates an empty `DFSchema`
pub fn empty() -> Self {
Self {
- fields: vec![],
- metadata: HashMap::new(),
+ inner: Arc::new(Schema::new([])),
+ field_qualifiers: vec![],
functional_dependencies: FunctionalDependencies::empty(),
}
}
- /// Create a new `DFSchema`
+ /// Create a new `DFSchema` from an Arrow schema
Review Comment:
it seems like this is not actual getting an arrow schema but instead a list
of `Field`s
--
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]
