Refactor Expr::Between to use a struct

datafusion-examples/examples/rewrite_expr.rs

@@ -18,7 +18,9 @@
 use arrow::datatypes::{DataType, Field, Schema, SchemaRef};
 use datafusion_common::{DataFusionError, Result};
 use datafusion_expr::expr_rewriter::{ExprRewritable, ExprRewriter};
-use datafusion_expr::{AggregateUDF, Expr, Filter, LogicalPlan, ScalarUDF, TableSource};
+use datafusion_expr::{
+    AggregateUDF, Between, Expr, Filter, LogicalPlan, ScalarUDF, TableSource,
+};
 use datafusion_optimizer::optimizer::Optimizer;
 use datafusion_optimizer::{utils, OptimizerConfig, OptimizerRule};
 use datafusion_sql::planner::{ContextProvider, SqlToRel};
@@ -99,12 +101,12 @@ struct MyExprRewriter {}
 impl ExprRewriter for MyExprRewriter {
     fn mutate(&mut self, expr: Expr) -> Result<Expr> {
         match expr {
-            Expr::Between {
-                negated,
+            Expr::Between(Between {
                 expr,
+                negated,
                 low,
                 high,
-            } => {
+            }) => {
                 let expr: Expr = expr.as_ref().clone();
                 let low: Expr = low.as_ref().clone();
                 let high: Expr = high.as_ref().clone();

datafusion/core/src/physical_plan/planner.rs

@@ -59,7 +59,7 @@ use arrow::compute::SortOptions;
 use arrow::datatypes::{Schema, SchemaRef};
 use async_trait::async_trait;
 use datafusion_common::{DFSchema, ScalarValue};
-use datafusion_expr::expr::{GroupingSet, Like};
+use datafusion_expr::expr::{Between, GroupingSet, Like};
 use datafusion_expr::expr_rewriter::unnormalize_cols;
 use datafusion_expr::utils::{expand_wildcard, expr_to_columns};
 use datafusion_expr::WindowFrameUnits;
@@ -258,12 +258,12 @@ fn create_physical_name(e: &Expr, is_first_expr: bool) -> Result<String> {
         Expr::ScalarSubquery(_) => Err(DataFusionError::NotImplemented(
             "Scalar subqueries are not yet supported in the physical plan".to_string(),
         )),
-        Expr::Between {
+        Expr::Between(Between {
             expr,
             negated,
             low,
             high,
-        } => {
+        }) => {
             let expr = create_physical_name(expr, false)?;
             let low = create_physical_name(low, false)?;
             let high = create_physical_name(high, false)?;

datafusion/expr/src/expr.rs

@@ -134,16 +134,7 @@ pub enum Expr {
         key: ScalarValue,
     },
     /// Whether an expression is between a given range.
-    Between {
-        /// The value to compare
-        expr: Box<Expr>,
-        /// Whether the expression is negated
-        negated: bool,
-        /// The low end of the range
-        low: Box<Expr>,
-        /// The high end of the range
-        high: Box<Expr>,
-    },
+    Between(Between),
     /// The CASE expression is similar to a series of nested if/else and there are two forms that
     /// can be used. The first form consists of a series of boolean "when" expressions with
     /// corresponding "then" expressions, and an optional "else" expression.
@@ -322,6 +313,31 @@ impl Like {
     }
 }
 
+/// BETWEEN expression
+#[derive(Clone, PartialEq, Eq, Hash)]
+pub struct Between {
+    /// The value to compare
+    pub expr: Box<Expr>,
+    /// Whether the expression is negated
+    pub negated: bool,
+    /// The low end of the range
+    pub low: Box<Expr>,
+    /// The high end of the range
+    pub high: Box<Expr>,
+}
+
+impl Between {
+    /// Create a new Between expression
+    pub fn new(expr: Box<Expr>, negated: bool, low: Box<Expr>, high: Box<Expr>) -> Self {
+        Self {
+            expr,
+            negated,
+            low,
+            high,
+        }
+    }
+}
+
 /// Grouping sets
 /// See https://www.postgresql.org/docs/current/queries-table-expressions.html#QUERIES-GROUPING-SETS
 /// for Postgres definition.
@@ -754,12 +770,12 @@ impl fmt::Debug for Expr {
                 }
                 Ok(())
             }
-            Expr::Between {
+            Expr::Between(Between {
                 expr,
                 negated,
                 low,
                 high,
-            } => {
+            }) => {
                 if *negated {
                     write!(f, "{:?} NOT BETWEEN {:?} AND {:?}", expr, low, high)
                 } else {
@@ -1136,12 +1152,12 @@ fn create_name(e: &Expr) -> Result<String> {
                 Ok(format!("{} IN ({:?})", expr, list))
             }
         }
-        Expr::Between {
+        Expr::Between(Between {
             expr,
             negated,
             low,
             high,
-        } => {
+        }) => {
             let expr = create_name(expr)?;
             let low = create_name(low)?;
             let high = create_name(high)?;

datafusion/expr/src/expr_rewriter.rs

@@ -17,7 +17,7 @@
 
 //! Expression rewriter
 
-use crate::expr::{Case, GroupingSet, Like};
+use crate::expr::{Between, Case, GroupingSet, Like};
 use crate::logical_plan::{Aggregate, Projection};
 use crate::utils::{from_plan, grouping_set_to_exprlist};
 use crate::{Expr, ExprSchemable, LogicalPlan};
@@ -173,17 +173,17 @@ impl ExprRewritable for Expr {
                 Expr::IsNotUnknown(rewrite_boxed(expr, rewriter)?)
             }
             Expr::Negative(expr) => Expr::Negative(rewrite_boxed(expr, rewriter)?),
-            Expr::Between {
+            Expr::Between(Between {
                 expr,
+                negated,
                 low,
                 high,
+            }) => Expr::Between(Between::new(
+                rewrite_boxed(expr, rewriter)?,
                 negated,
-            } => Expr::Between {
-                expr: rewrite_boxed(expr, rewriter)?,
-                low: rewrite_boxed(low, rewriter)?,
-                high: rewrite_boxed(high, rewriter)?,
-                negated,
-            },
+                rewrite_boxed(low, rewriter)?,
+                rewrite_boxed(high, rewriter)?,
+            )),
             Expr::Case(case) => {
                 let expr = rewrite_option_box(case.expr, rewriter)?;
                 let when_then_expr = case

datafusion/expr/src/expr_schema.rs

@@ -15,7 +15,7 @@
 // specific language governing permissions and limitations
 // under the License.
 
-use super::{Expr, Like};
+use super::{Between, Expr, Like};
 use crate::field_util::get_indexed_field;
 use crate::type_coercion::binary::binary_operator_data_type;
 use crate::{aggregate_function, function, window_function};
@@ -160,8 +160,8 @@ impl ExprSchemable for Expr {
             | Expr::Not(expr)
             | Expr::Negative(expr)
             | Expr::Sort { expr, .. }
-            | Expr::Between { expr, .. }
             | Expr::InList { expr, .. } => expr.nullable(input_schema),
+            Expr::Between(Between { expr, .. }) => expr.nullable(input_schema),
             Expr::Column(c) => input_schema.nullable(c),
             Expr::Literal(value) => Ok(value.is_null()),
             Expr::Case(case) => {

datafusion/expr/src/expr_visitor.rs

@@ -17,7 +17,7 @@
 
 //! Expression visitor
 
-use crate::{expr::GroupingSet, Expr, Like};
+use crate::{expr::GroupingSet, Between, Expr, Like};
 use datafusion_common::Result;
 
 /// Controls how the visitor recursion should proceed.
@@ -146,9 +146,9 @@ impl ExprVisitable for Expr {
                 let visitor = expr.accept(visitor)?;
                 pattern.accept(visitor)
             }
-            Expr::Between {
+            Expr::Between(Between {
                 expr, low, high, ..
-            } => {
+            }) => {
                 let visitor = expr.accept(visitor)?;
                 let visitor = low.accept(visitor)?;
                 high.accept(visitor)

datafusion/expr/src/lib.rs

@@ -56,7 +56,7 @@ pub use accumulator::{Accumulator, AggregateState};
 pub use aggregate_function::AggregateFunction;
 pub use built_in_function::BuiltinScalarFunction;
 pub use columnar_value::{ColumnarValue, NullColumnarValue};
-pub use expr::{Case, Expr, GroupingSet, Like};
+pub use expr::{Between, Case, Expr, GroupingSet, Like};
 pub use expr_fn::*;
 pub use expr_schema::ExprSchemable;
 pub use function::{

datafusion/optimizer/src/simplify_expressions.rs

@@ -25,6 +25,7 @@ use arrow::error::ArrowError;
 use arrow::record_batch::RecordBatch;
 use datafusion_common::{DFSchema, DataFusionError, Result, ScalarValue};
 use datafusion_expr::{
+    expr::Between,
     expr_fn::{and, or},
     expr_rewriter::{ExprRewritable, ExprRewriter, RewriteRecursion},
     lit,
@@ -245,17 +246,12 @@ fn negate_clause(expr: Expr) -> Expr {
         } => expr.in_list(list, !negated),
         // not (A between B and C) ===> (A not between B and C)
         // not (A not between B and C) ===> (A between B and C)
-        Expr::Between {
-            expr,
-            negated,
-            low,
-            high,
-        } => Expr::Between {
-            expr,
-            negated: !negated,
-            low,
-            high,
-        },
+        Expr::Between(between) => Expr::Between(Between::new(
+            between.expr,
+            !between.negated,
+            between.low,
+            between.high,
+        )),
         // use not clause
         _ => Expr::Not(Box::new(expr)),
     }
@@ -949,18 +945,16 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a, S> {
 
             // a between 3 and 5  -->  a >= 3 AND a <=5
             // a not between 3 and 5  -->  a < 3 OR a > 5
-            Between {
-                expr,
-                low,
-                high,
-                negated,
-            } => {
-                if negated {
-                    let l = *expr.clone();
-                    let r = *expr;
-                    or(l.lt(*low), r.gt(*high))
+            Between(between) => {
+                if between.negated {
+                    let l = *between.expr.clone();
+                    let r = *between.expr;
+                    or(l.lt(*between.low), r.gt(*between.high))
                 } else {
-                    and(expr.clone().gt_eq(*low), expr.lt_eq(*high))
+                    and(
+                        between.expr.clone().gt_eq(*between.low),
+                        between.expr.lt_eq(*between.high),
+                    )
                 }
             }
             expr => {
@@ -1908,12 +1902,12 @@ mod tests {
         // ( c1 BETWEEN Int32(0) AND Int32(10) ) OR Boolean(NULL)
         // it can be either NULL or  TRUE depending on the value of `c1 BETWEEN Int32(0) AND Int32(10)`
         // and should not be rewritten
-        let expr = Expr::Between {
-            expr: Box::new(col("c1")),
-            negated: false,
-            low: Box::new(lit(0)),
-            high: Box::new(lit(10)),
-        };
+        let expr = Expr::Between(Between::new(
+            Box::new(col("c1")),
+            false,
+            Box::new(lit(0)),
+            Box::new(lit(10)),
+        ));
         let expr = expr.or(lit_bool_null());
         let result = simplify(expr);
 
@@ -1946,12 +1940,12 @@ mod tests {
         // c1 BETWEEN Int32(0) AND Int32(10) AND Boolean(NULL)
         // it can be either NULL or FALSE depending on the value of `c1 BETWEEN Int32(0) AND Int32(10)`
         // and the Boolean(NULL) should remain
-        let expr = Expr::Between {
-            expr: Box::new(col("c1")),
-            negated: false,
-            low: Box::new(lit(0)),
-            high: Box::new(lit(10)),
-        };
+        let expr = Expr::Between(Between::new(
+            Box::new(col("c1")),
+            false,
+            Box::new(lit(0)),
+            Box::new(lit(10)),
+        ));
         let expr = expr.and(lit_bool_null());
         let result = simplify(expr);
 
@@ -1965,24 +1959,24 @@ mod tests {
     #[test]
     fn simplify_expr_between() {
         // c2 between 3 and 4 is c2 >= 3 and c2 <= 4
-        let expr = Expr::Between {
-            expr: Box::new(col("c2")),
-            negated: false,
-            low: Box::new(lit(3)),
-            high: Box::new(lit(4)),
-        };
+        let expr = Expr::Between(Between::new(
+            Box::new(col("c2")),
+            false,
+            Box::new(lit(3)),
+            Box::new(lit(4)),
+        ));
         assert_eq!(
             simplify(expr),
             and(col("c2").gt_eq(lit(3)), col("c2").lt_eq(lit(4)))
         );
 
         // c2 not between 3 and 4 is c2 < 3 or c2 > 4
-        let expr = Expr::Between {
-            expr: Box::new(col("c2")),
-            negated: true,
-            low: Box::new(lit(3)),
-            high: Box::new(lit(4)),
-        };
+        let expr = Expr::Between(Between::new(
+            Box::new(col("c2")),
+            true,
+            Box::new(lit(3)),
+            Box::new(lit(4)),
+        ));
         assert_eq!(
             simplify(expr),
             or(col("c2").lt(lit(3)), col("c2").gt(lit(4)))
@@ -2554,12 +2548,12 @@ mod tests {
     #[test]
     fn simplify_not_between() {
         let table_scan = test_table_scan();
-        let qual = Expr::Between {
-            expr: Box::new(col("d")),
-            negated: false,
-            low: Box::new(lit(1)),
-            high: Box::new(lit(10)),
-        };
+        let qual = Expr::Between(Between::new(
+            Box::new(col("d")),
+            false,
+            Box::new(lit(1)),
+            Box::new(lit(10)),
+        ));
 
         let plan = LogicalPlanBuilder::from(table_scan)
             .filter(qual.not())
@@ -2575,12 +2569,12 @@ mod tests {
     #[test]
     fn simplify_not_not_between() {
         let table_scan = test_table_scan();
-        let qual = Expr::Between {
-            expr: Box::new(col("d")),
-            negated: true,
-            low: Box::new(lit(1)),
-            high: Box::new(lit(10)),
-        };
+        let qual = Expr::Between(Between::new(
+            Box::new(col("d")),
+            true,
+            Box::new(lit(1)),
+            Box::new(lit(10)),
+        ));
 
         let plan = LogicalPlanBuilder::from(table_scan)
             .filter(qual.not())