From 94d53aa98881b8a2b3d35bd10cbff51dd5e25c60 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Thu, 31 Oct 2024 15:11:17 -0400 Subject: [PATCH 1/2] Minor: make `Expr::volatile` infallible --- datafusion/expr/src/expr.rs | 8 +++++++- datafusion/optimizer/src/push_down_filter.rs | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs index bda4d7ae3d7fa..dce76aabe6f09 100644 --- a/datafusion/expr/src/expr.rs +++ b/datafusion/expr/src/expr.rs @@ -1574,8 +1574,14 @@ impl Expr { /// Returns true if the expression is volatile, i.e. whether it can return different /// results when evaluated multiple times with the same input. - pub fn is_volatile(&self) -> Result { + /// + /// For example the function call `RANDOM()` is volatile as each call will + /// return a different value. + /// + /// See [`Volatility`] for more information. + pub fn is_volatile(&self) -> bool { self.exists(|expr| Ok(expr.is_volatile_node())) + .unwrap() } /// Recursively find all [`Expr::Placeholder`] expressions, and diff --git a/datafusion/optimizer/src/push_down_filter.rs b/datafusion/optimizer/src/push_down_filter.rs index 1d0fc207cb51d..269ce29100746 100644 --- a/datafusion/optimizer/src/push_down_filter.rs +++ b/datafusion/optimizer/src/push_down_filter.rs @@ -1200,7 +1200,7 @@ fn rewrite_projection( (qualified_name(qualifier, field.name()), expr) }) - .partition(|(_, value)| value.is_volatile().unwrap_or(true)); + .partition(|(_, value)| value.is_volatile()); let mut push_predicates = vec![]; let mut keep_predicates = vec![]; From 352c87b794caf8aa5bdb7c8af50299d58776d85f Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Thu, 31 Oct 2024 15:15:00 -0400 Subject: [PATCH 2/2] cmt --- datafusion/expr/src/expr.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs index dce76aabe6f09..a9c183952fc72 100644 --- a/datafusion/expr/src/expr.rs +++ b/datafusion/expr/src/expr.rs @@ -1580,8 +1580,7 @@ impl Expr { /// /// See [`Volatility`] for more information. pub fn is_volatile(&self) -> bool { - self.exists(|expr| Ok(expr.is_volatile_node())) - .unwrap() + self.exists(|expr| Ok(expr.is_volatile_node())).unwrap() } /// Recursively find all [`Expr::Placeholder`] expressions, and