Remove GetField usage (see apache#628)

Author: Igosuki

datafusion/src/lib.rs

@@ -225,7 +225,6 @@ pub mod physical_plan;
 pub mod prelude;
 pub mod scalar;
 pub mod sql;
-mod utils;
 pub mod variable;
 
 // re-export dependencies from arrow-rs to minimise version maintenance for crate users

datafusion/src/logical_plan/dfschema.rs

@@ -25,7 +25,6 @@ use std::sync::Arc;
 use crate::error::{DataFusionError, Result};
 use crate::logical_plan::Column;
 
-use crate::utils::get_field;
 use arrow::datatypes::{DataType, Field, Schema, SchemaRef};
 use std::fmt::{Display, Formatter};
 
@@ -161,11 +160,7 @@ impl DFSchema {
                 // field to lookup is qualified.
                 // current field is qualified and not shared between relations, compare both
                 // qualifer and name.
-                (Some(q), Some(field_q)) => {
-                    (q == field_q && field.name() == name)
-                        || (q == field.name()
-                            && get_field(field.field.data_type(), name).is_ok())
-                }
+                (Some(q), Some(field_q)) => q == field_q && field.name() == name,
                 // field to lookup is qualified but current field is unqualified.
                 (Some(_), None) => false,
                 // field to lookup is unqualified, no need to compare qualifier

datafusion/src/logical_plan/expr.rs

@@ -25,7 +25,6 @@ use crate::physical_plan::{
     aggregates, expressions::binary_operator_data_type, functions, udf::ScalarUDF,
     window_functions,
 };
-use crate::utils::get_field;
 use crate::{physical_plan::udaf::AggregateUDF, scalar::ScalarValue};
 use aggregates::{AccumulatorFunctionImplementation, StateTypeFunction};
 use arrow::{compute::can_cast_types, datatypes::DataType};
@@ -245,13 +244,6 @@ pub enum Expr {
     IsNull(Box<Expr>),
     /// arithmetic negation of an expression, the operand must be of a signed numeric data type
     Negative(Box<Expr>),
-    /// Returns the field of a [`StructArray`] by name
-    GetField {
-        /// the expression to take the field from
-        expr: Box<Expr>,
-        /// The name of the field to take
-        name: String,
-    },
     /// Whether an expression is between a given range.
     Between {
         /// The value to compare
@@ -440,10 +432,6 @@ impl Expr {
             Expr::Wildcard => Err(DataFusionError::Internal(
                 "Wildcard expressions are not valid in a logical query plan".to_owned(),
             )),
-            Expr::GetField { ref expr, name } => {
-                let data_type = expr.get_type(schema)?;
-                get_field(&data_type, name).map(|x| x.data_type().clone())
-            }
         }
     }
 
@@ -499,10 +487,6 @@ impl Expr {
             Expr::Wildcard => Err(DataFusionError::Internal(
                 "Wildcard expressions are not valid in a logical query plan".to_owned(),
             )),
-            Expr::GetField { ref expr, name } => {
-                let data_type = expr.get_type(input_schema)?;
-                get_field(&data_type, name).map(|x| x.is_nullable())
-            }
         }
     }
 
@@ -643,14 +627,6 @@ impl Expr {
         Expr::IsNotNull(Box::new(self))
     }
 
-    /// Returns the values of the field `name` from an expression returning a `Struct`
-    pub fn get_field<I: Into<String>>(self, name: I) -> Expr {
-        Expr::GetField {
-            expr: Box::new(self),
-            name: name.into(),
-        }
-    }
-
     /// Create a sort expression from an existing expression.
     ///
     /// ```
@@ -786,7 +762,6 @@ impl Expr {
                     .try_fold(visitor, |visitor, arg| arg.accept(visitor))
             }
             Expr::Wildcard => Ok(visitor),
-            Expr::GetField { ref expr, .. } => expr.accept(visitor),
         }?;
 
         visitor.post_visit(self)
@@ -944,10 +919,6 @@ impl Expr {
                 negated,
             },
             Expr::Wildcard => Expr::Wildcard,
-            Expr::GetField { expr, name } => Expr::GetField {
-                expr: rewrite_boxed(expr, rewriter)?,
-                name,
-            },
         };
 
         // now rewrite this expression itself
@@ -1709,7 +1680,6 @@ impl fmt::Debug for Expr {
                 }
             }
             Expr::Wildcard => write!(f, "*"),
-            Expr::GetField { ref expr, name } => write!(f, "({:?}).{}", expr, name),
         }
     }
 }
@@ -1786,10 +1756,6 @@ fn create_name(e: &Expr, input_schema: &DFSchema) -> Result<String> {
             let expr = create_name(expr, input_schema)?;
             Ok(format!("{} IS NOT NULL", expr))
         }
-        Expr::GetField { expr, name } => {
-            let expr = create_name(expr, input_schema)?;
-            Ok(format!("{}.{}", expr, name))
-        }
         Expr::ScalarFunction { fun, args, .. } => {
             create_function_name(&fun.to_string(), false, args, input_schema)
         }
@@ -1911,12 +1877,6 @@ mod tests {
         );
     }
 
-    #[test]
-    fn display_get_field() {
-        let col_null = col("col1").get_field("name");
-        assert_eq!(format!("{:?}", col_null), "(#col1).name");
-    }
-
     #[derive(Default)]
     struct RecordingRewriter {
         v: Vec<String>,

datafusion/src/optimizer/utils.rs

@@ -79,7 +79,6 @@ impl ExpressionVisitor for ColumnNameVisitor<'_> {
             Expr::AggregateUDF { .. } => {}
             Expr::InList { .. } => {}
             Expr::Wildcard => {}
-            Expr::GetField { .. } => {}
         }
         Ok(Recursion::Continue(self))
     }
@@ -306,7 +305,6 @@ pub fn expr_sub_expressions(expr: &Expr) -> Result<Vec<Expr>> {
         Expr::Wildcard { .. } => Err(DataFusionError::Internal(
             "Wildcard expressions are not valid in a logical query plan".to_owned(),
         )),
-        Expr::GetField { expr, .. } => Ok(vec![expr.as_ref().to_owned()]),
     }
 }
 
@@ -322,10 +320,6 @@ pub fn rewrite_expression(expr: &Expr, expressions: &[Expr]) -> Result<Expr> {
         }),
         Expr::IsNull(_) => Ok(Expr::IsNull(Box::new(expressions[0].clone()))),
         Expr::IsNotNull(_) => Ok(Expr::IsNotNull(Box::new(expressions[0].clone()))),
-        Expr::GetField { expr: _, name } => Ok(Expr::GetField {
-            expr: Box::new(expressions[0].clone()),
-            name: name.clone(),
-        }),
         Expr::ScalarFunction { fun, .. } => Ok(Expr::ScalarFunction {
             fun: fun.clone(),
             args: expressions.to_vec(),

datafusion/src/physical_plan/expressions/get_field.rs

@@ -33,7 +33,6 @@ mod cast;
 mod coercion;
 mod column;
 mod count;
-mod get_field;
 mod in_list;
 mod is_not_null;
 mod is_null;
@@ -57,7 +56,6 @@ pub use cast::{
 };
 pub use column::{col, Column};
 pub use count::Count;
-pub use get_field::{get_field, GetFieldExpr};
 pub use in_list::{in_list, InListExpr};
 pub use is_not_null::{is_not_null, IsNotNullExpr};
 pub use is_null::{is_null, IsNullExpr};

datafusion/src/physical_plan/expressions/mod.rs

@@ -138,10 +138,6 @@ fn create_physical_name(e: &Expr, is_first_expr: bool) -> Result<String> {
             let expr = create_physical_name(expr, false)?;
             Ok(format!("{} IS NOT NULL", expr))
         }
-        Expr::GetField { expr, name } => {
-            let expr = physical_name(expr, input_schema)?;
-            Ok(format!("{}.{}", expr, name))
-        }
         Expr::ScalarFunction { fun, args, .. } => {
             create_function_physical_name(&fun.to_string(), false, args)
         }
@@ -964,10 +960,6 @@ impl DefaultPhysicalPlanner {
             Expr::IsNotNull(expr) => expressions::is_not_null(
                 self.create_physical_expr(expr, input_dfschema, input_schema, ctx_state)?,
             ),
-            Expr::GetField { expr, name } => expressions::get_field(
-                self.create_physical_expr(expr, input_dfschema, input_schema, ctx_state)?,
-                name.clone(),
-            ),
             Expr::ScalarFunction { fun, args } => {
                 let physical_args = args
                     .iter()

datafusion/src/physical_plan/planner.rs

@@ -81,22 +81,6 @@ pub struct SqlToRel<'a, S: ContextProvider> {
     schema_provider: &'a S,
 }
 
-fn plan_compound(mut identifiers: Vec<String>) -> Expr {
-    if &identifiers[0][0..1] == "@" {
-        Expr::ScalarVariable(identifiers)
-    } else if identifiers.len() == 2 {
-        // "table.column"
-        let name = identifiers.pop().unwrap();
-        let relation = Some(identifiers.pop().unwrap());
-        Expr::Column(Column { relation, name })
-    } else {
-        // "table.column.field..."
-        let name = identifiers.pop().unwrap();
-        let expr = Box::new(plan_compound(identifiers));
-        Expr::GetField { expr, name }
-    }
-}
-
 impl<'a, S: ContextProvider> SqlToRel<'a, S> {
     /// Create a new query planner
     pub fn new(schema_provider: &'a S) -> Self {
@@ -1110,8 +1094,23 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
             }
 
             SQLExpr::CompoundIdentifier(ids) => {
-                let var_names = ids.iter().map(|x| x.value.clone()).collect::<Vec<_>>();
-                Ok(plan_compound(var_names))
+                let mut var_names = vec![];
+                for id in ids {
+                    var_names.push(id.value.clone());
+                }
+                if &var_names[0][0..1] == "@" {
+                    Ok(Expr::ScalarVariable(var_names))
+                } else if var_names.len() == 2 {
+                    // table.column identifier
+                    let name = var_names.pop().unwrap();
+                    let relation = Some(var_names.pop().unwrap());
+                    Ok(Expr::Column(Column { relation, name }))
+                } else {
+                    Err(DataFusionError::NotImplemented(format!(
+                        "Unsupported compound identifier '{:?}'",
+                        var_names,
+                    )))
+                }
             }
 
             SQLExpr::Wildcard => Ok(Expr::Wildcard),

datafusion/src/sql/planner.rs

@@ -368,10 +368,6 @@ where
                 Ok(expr.clone())
             }
             Expr::Wildcard => Ok(Expr::Wildcard),
-            Expr::GetField { expr, name } => Ok(Expr::GetField {
-                expr: Box::new(clone_with_replacement(expr.as_ref(), replacement_fn)?),
-                name: name.clone(),
-            }),
         },
     }
 }