alamb commented on code in PR #3969:
URL: https://github.com/apache/arrow-datafusion/pull/3969#discussion_r1006725058
##########
datafusion/physical-expr/src/physical_expr.rs:
##########
@@ -66,6 +67,14 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug {
fn expr_stats(&self) -> Arc<dyn PhysicalExprStats> {
Arc::new(BasicExpressionStats {})
}
+ /// Get a list of child PhysicalExpr that provide the input for this expr.
Review Comment:
👍 these are foundational changes needed to do PhysicalExpr rewrites
##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -93,6 +94,36 @@ impl PhysicalExpr for CastExpr {
let value = self.expr.evaluate(batch)?;
cast_column(&value, &self.cast_type, &self.cast_options)
}
+
+ fn children(&self) -> Vec<Arc<dyn PhysicalExpr>> {
+ vec![self.expr.clone()]
+ }
+
+ fn with_new_children(
+ self: Arc<Self>,
+ children: Vec<Arc<dyn PhysicalExpr>>,
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ Ok(Arc::new(CastExpr::new(
Review Comment:
recommend assert / error check for children.len() == 1
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -286,6 +290,89 @@ impl PhysicalExpr for CaseExpr {
self.case_when_no_expr(batch)
}
}
+
+ fn children(&self) -> Vec<Arc<dyn PhysicalExpr>> {
+ let mut chileren = vec![];
+ match &self.expr {
+ Some(expr) => chileren.push(expr.clone()),
+ None => chileren.push(Arc::new(NoOp::new())),
+ }
+ self.when_then_expr.iter().for_each(|(cond, value)| {
+ chileren.push(cond.clone());
+ chileren.push(value.clone());
+ });
+
+ match &self.else_expr {
+ Some(expr) => chileren.push(expr.clone()),
+ None => chileren.push(Arc::new(NoOp::new())),
+ }
+ chileren
+ }
+
+ // For physical CaseExpr, we do not allow modifying children size
+ fn with_new_children(
+ self: Arc<Self>,
+ children: Vec<Arc<dyn PhysicalExpr>>,
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ if children.len() != self.children().len() {
+ Err(DataFusionError::Internal(
+ "CaseExpr: Wrong number of children".to_string(),
+ ))
+ } else {
+ assert_eq!(children.len() % 2, 0);
+ let expr = match
children[0].clone().as_any().downcast_ref::<NoOp>() {
+ Some(_) => None,
+ _ => Some(children[0].clone()),
+ };
+ let else_expr = match children[children.len() - 1]
+ .clone()
+ .as_any()
+ .downcast_ref::<NoOp>()
+ {
+ Some(_) => None,
+ _ => Some(children[children.len() - 1].clone()),
+ };
+
+ let branches = children[1..children.len() - 1].to_vec();
+ let mut when_then_expr: Vec<WhenThen> = vec![];
+ for (prev, next) in branches.into_iter().tuples() {
+ when_then_expr.push((prev, next));
+ }
+ Ok(Arc::new(CaseExpr::try_new(
+ expr,
+ when_then_expr,
+ else_expr,
+ )?))
+ }
+ }
+}
+
+impl PartialEq<dyn Any> for CaseExpr {
+ fn eq(&self, other: &dyn Any) -> bool {
+ down_cast_any_ref(other)
+ .downcast_ref::<Self>()
+ .map(|x| {
+ let expr_eq = match (&self.expr, &x.expr) {
+ (Some(expr1), Some(expr2)) => expr1.eq(expr2),
+ (None, None) => true,
+ _ => false,
Review Comment:
You could also do `return false` here rather than checking `expr_eq` again
later
##########
datafusion/physical-expr/src/expressions/no_op.rs:
##########
@@ -0,0 +1,87 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Literal expressions for physical operations
Review Comment:
```suggestion
//! NoOp placeholder for physical operations
```
##########
datafusion/physical-expr/src/expressions/no_op.rs:
##########
@@ -0,0 +1,87 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Literal expressions for physical operations
+
+use std::any::Any;
+use std::sync::Arc;
+
+use arrow::{
+ datatypes::{DataType, Schema},
+ record_batch::RecordBatch,
+};
+
+use crate::physical_expr::down_cast_any_ref;
+use crate::PhysicalExpr;
+use datafusion_common::Result;
+use datafusion_expr::ColumnarValue;
+
+/// A place holder expressions, can not be evaluated
+#[derive(Debug, PartialEq, Eq, Default)]
+pub struct NoOp {}
+
+impl NoOp {
+ /// Create a NoOp expression
+ pub fn new() -> Self {
+ Self {}
+ }
+}
+
+impl std::fmt::Display for NoOp {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "NoOp")
+ }
+}
+
+impl PhysicalExpr for NoOp {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, _input_schema: &Schema) -> Result<DataType> {
+ Ok(DataType::Null)
+ }
+
+ fn nullable(&self, _input_schema: &Schema) -> Result<bool> {
+ Ok(true)
+ }
+
+ fn evaluate(&self, _batch: &RecordBatch) -> Result<ColumnarValue> {
+ unimplemented!("NoOp::evaluate");
Review Comment:
I recommend returning an error here rather than panic (which is what
`unimplemented!` will do)
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -145,4 +145,26 @@ impl PhysicalExpr for ScalarFunctionExpr {
let fun = self.fun.as_ref();
(fun)(&inputs)
}
+
+ fn children(&self) -> Vec<Arc<dyn PhysicalExpr>> {
+ self.args.clone()
+ }
+
+ fn with_new_children(
+ self: Arc<Self>,
+ children: Vec<Arc<dyn PhysicalExpr>>,
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ Ok(Arc::new(ScalarFunctionExpr::new(
+ &self.name,
+ self.fun.clone(),
+ children,
+ self.return_type(),
+ )))
+ }
+}
+
+impl PartialEq<dyn Any> for ScalarFunctionExpr {
+ fn eq(&self, _other: &dyn Any) -> bool {
+ false
Review Comment:
This seems incorrect / unfinshed -- shouldn't it be checking the contents of
the `other`?
##########
datafusion/physical-expr/src/physical_expr.rs:
##########
@@ -128,6 +137,232 @@ impl PhysicalExprStats for BasicExpressionStats {
}
}
+/// a Trait for marking tree node types that are rewritable
Review Comment:
I recommend putting the rewriting traits into some other module, such as
`datafusion/physical-expr/src/rewrite.rs` or example
We can do it as a follow on PR too
##########
datafusion/physical-expr/src/expressions/no_op.rs:
##########
@@ -0,0 +1,87 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Literal expressions for physical operations
+
+use std::any::Any;
+use std::sync::Arc;
+
+use arrow::{
+ datatypes::{DataType, Schema},
+ record_batch::RecordBatch,
+};
+
+use crate::physical_expr::down_cast_any_ref;
+use crate::PhysicalExpr;
+use datafusion_common::Result;
+use datafusion_expr::ColumnarValue;
+
+/// A place holder expressions, can not be evaluated
Review Comment:
```suggestion
/// A place holder expressions, can not be evaluated.
///
/// Used in some cases where an `Arc<dyn PhysicalExpr>` is needed, such as
`children()`
```
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -649,6 +650,30 @@ impl PhysicalExpr for BinaryExpr {
right: Arc::clone(self.right()),
})
}
+
+ fn children(&self) -> Vec<Arc<dyn PhysicalExpr>> {
+ vec![self.left.clone(), self.right.clone()]
+ }
+
+ fn with_new_children(
+ self: Arc<Self>,
+ children: Vec<Arc<dyn PhysicalExpr>>,
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ Ok(Arc::new(BinaryExpr::new(
Review Comment:
I suggest we should also `assert_eq!(children.len(), 2)` to catch bugs
earlier
##########
datafusion/physical-expr/src/expressions/datetime.rs:
##########
@@ -43,6 +44,7 @@ pub struct DateTimeIntervalExpr {
lhs: Arc<dyn PhysicalExpr>,
op: Operator,
rhs: Arc<dyn PhysicalExpr>,
+ input_schema: Schema,
Review Comment:
```suggestion
// TODO: move type checking to the planning phase and not in the
physical expr
// so we can remove this
input_schema: Schema,
```
This is unfortunate -- I see you need to do it so you can call `try_new()`
-- but really I think DataFusion should have done this type checking in the
planning phase rather than at the physical expr stage -- maybe I'll file a
ticket to improve that.
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -286,6 +290,89 @@ impl PhysicalExpr for CaseExpr {
self.case_when_no_expr(batch)
}
}
+
+ fn children(&self) -> Vec<Arc<dyn PhysicalExpr>> {
+ let mut chileren = vec![];
Review Comment:
```suggestion
let mut children = vec![];
```
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -286,6 +290,89 @@ impl PhysicalExpr for CaseExpr {
self.case_when_no_expr(batch)
}
}
+
+ fn children(&self) -> Vec<Arc<dyn PhysicalExpr>> {
+ let mut chileren = vec![];
+ match &self.expr {
+ Some(expr) => chileren.push(expr.clone()),
+ None => chileren.push(Arc::new(NoOp::new())),
+ }
+ self.when_then_expr.iter().for_each(|(cond, value)| {
+ chileren.push(cond.clone());
+ chileren.push(value.clone());
+ });
+
+ match &self.else_expr {
+ Some(expr) => chileren.push(expr.clone()),
+ None => chileren.push(Arc::new(NoOp::new())),
+ }
+ chileren
+ }
+
+ // For physical CaseExpr, we do not allow modifying children size
+ fn with_new_children(
+ self: Arc<Self>,
+ children: Vec<Arc<dyn PhysicalExpr>>,
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ if children.len() != self.children().len() {
+ Err(DataFusionError::Internal(
+ "CaseExpr: Wrong number of children".to_string(),
+ ))
+ } else {
+ assert_eq!(children.len() % 2, 0);
+ let expr = match
children[0].clone().as_any().downcast_ref::<NoOp>() {
+ Some(_) => None,
+ _ => Some(children[0].clone()),
+ };
+ let else_expr = match children[children.len() - 1]
Review Comment:
i would find it easier to read if the `else_expr` handling was after the
`when_then_expr` handling to mirror the `children()` but I think this is fine
too
##########
datafusion/physical-expr/src/expressions/in_list.rs:
##########
@@ -52,16 +54,27 @@ use datafusion_expr::ColumnarValue;
static OPTIMIZER_INSET_THRESHOLD: usize = 30;
/// InList
-#[derive(Debug)]
pub struct InListExpr {
expr: Arc<dyn PhysicalExpr>,
list: Vec<Arc<dyn PhysicalExpr>>,
negated: bool,
set: Option<InSet>,
+ input_schema: Schema,
Review Comment:
Why does this struct need `input_schema`? It seems to me it is not needed
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -286,6 +290,89 @@ impl PhysicalExpr for CaseExpr {
self.case_when_no_expr(batch)
}
}
+
+ fn children(&self) -> Vec<Arc<dyn PhysicalExpr>> {
+ let mut chileren = vec![];
+ match &self.expr {
+ Some(expr) => chileren.push(expr.clone()),
+ None => chileren.push(Arc::new(NoOp::new())),
+ }
+ self.when_then_expr.iter().for_each(|(cond, value)| {
+ chileren.push(cond.clone());
+ chileren.push(value.clone());
+ });
+
+ match &self.else_expr {
+ Some(expr) => chileren.push(expr.clone()),
+ None => chileren.push(Arc::new(NoOp::new())),
+ }
+ chileren
+ }
+
+ // For physical CaseExpr, we do not allow modifying children size
Review Comment:
👍
##########
datafusion/physical-expr/src/expressions/no_op.rs:
##########
@@ -0,0 +1,87 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Literal expressions for physical operations
+
+use std::any::Any;
+use std::sync::Arc;
+
+use arrow::{
+ datatypes::{DataType, Schema},
+ record_batch::RecordBatch,
+};
+
+use crate::physical_expr::down_cast_any_ref;
+use crate::PhysicalExpr;
+use datafusion_common::Result;
+use datafusion_expr::ColumnarValue;
+
+/// A place holder expressions, can not be evaluated
+#[derive(Debug, PartialEq, Eq, Default)]
+pub struct NoOp {}
+
+impl NoOp {
+ /// Create a NoOp expression
+ pub fn new() -> Self {
+ Self {}
+ }
+}
+
+impl std::fmt::Display for NoOp {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ write!(f, "NoOp")
+ }
+}
+
+impl PhysicalExpr for NoOp {
+ /// Return a reference to Any that can be used for downcasting
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn data_type(&self, _input_schema: &Schema) -> Result<DataType> {
+ Ok(DataType::Null)
Review Comment:
🤔 since this acts like a `PhysicalExpr::Literal` for `ScalarValue::Null` I
wonder if you considered using `PhysicalExpr::Literal` directly instead (for
CaseExpr)?
##########
datafusion/physical-expr/src/physical_expr.rs:
##########
@@ -128,6 +137,232 @@ impl PhysicalExprStats for BasicExpressionStats {
}
}
+/// a Trait for marking tree node types that are rewritable
+pub trait TreeNodeRewritable: Clone {
+ /// Transform the tree node using the given [TreeNodeRewriter]
+ /// It performs a depth first walk of an node and its children.
+ ///
+ /// For an node tree such as
+ /// ```text
+ /// ParentNode
+ /// left: ChildNode1
+ /// right: ChildNode2
+ /// ```
+ ///
+ /// The nodes are visited using the following order
+ /// ```text
+ /// pre_visit(ParentNode)
+ /// pre_visit(ChildNode1)
+ /// mutatate(ChildNode1)
+ /// pre_visit(ChildNode2)
+ /// mutate(ChildNode2)
+ /// mutate(ParentNode)
+ /// ```
+ ///
+ /// If an Err result is returned, recursion is stopped immediately
+ ///
+ /// If [`false`] is returned on a call to pre_visit, no
+ /// children of that node are visited, nor is mutate
+ /// called on that node
+ ///
+ fn transform_using<R: TreeNodeRewriter<Self>>(
+ self,
+ rewriter: &mut R,
+ ) -> Result<Self> {
+ let need_mutate = match rewriter.pre_visit(&self)? {
+ RewriteRecursion::Mutate => return rewriter.mutate(self),
+ RewriteRecursion::Stop => return Ok(self),
+ RewriteRecursion::Continue => true,
+ RewriteRecursion::Skip => false,
+ };
+
+ let after_op_children =
+ self.map_children(|node| node.transform_using(rewriter))?;
+
+ // now rewrite this node itself
+ if need_mutate {
+ rewriter.mutate(after_op_children)
+ } else {
+ Ok(after_op_children)
+ }
+ }
+
+ /// Convenience utils for writing optimizers rule: recursively apply the
given `op` to the node tree.
+ /// When `op` does not apply to a given node, it is left uncshanged.
Review Comment:
```suggestion
/// When `op` does not apply to a given node, it is left unchanged.
```
##########
datafusion/physical-expr/src/physical_expr.rs:
##########
@@ -128,6 +137,232 @@ impl PhysicalExprStats for BasicExpressionStats {
}
}
+/// a Trait for marking tree node types that are rewritable
+pub trait TreeNodeRewritable: Clone {
+ /// Transform the tree node using the given [TreeNodeRewriter]
+ /// It performs a depth first walk of an node and its children.
+ ///
+ /// For an node tree such as
+ /// ```text
+ /// ParentNode
+ /// left: ChildNode1
+ /// right: ChildNode2
+ /// ```
+ ///
+ /// The nodes are visited using the following order
+ /// ```text
+ /// pre_visit(ParentNode)
Review Comment:
I think these examples need to be change to use the terms `transform_down`
and `transform_up` and `transform`
I also think it would be nice to use the same terms here as are used in the
rewriter (previsit / postvisit) but I realize they don't quite match up.
##########
datafusion/physical-expr/src/physical_expr.rs:
##########
@@ -128,6 +137,232 @@ impl PhysicalExprStats for BasicExpressionStats {
}
}
+/// a Trait for marking tree node types that are rewritable
+pub trait TreeNodeRewritable: Clone {
+ /// Transform the tree node using the given [TreeNodeRewriter]
+ /// It performs a depth first walk of an node and its children.
+ ///
+ /// For an node tree such as
+ /// ```text
+ /// ParentNode
+ /// left: ChildNode1
+ /// right: ChildNode2
+ /// ```
+ ///
+ /// The nodes are visited using the following order
+ /// ```text
+ /// pre_visit(ParentNode)
+ /// pre_visit(ChildNode1)
+ /// mutatate(ChildNode1)
+ /// pre_visit(ChildNode2)
+ /// mutate(ChildNode2)
+ /// mutate(ParentNode)
+ /// ```
+ ///
+ /// If an Err result is returned, recursion is stopped immediately
+ ///
+ /// If [`false`] is returned on a call to pre_visit, no
+ /// children of that node are visited, nor is mutate
+ /// called on that node
+ ///
+ fn transform_using<R: TreeNodeRewriter<Self>>(
+ self,
+ rewriter: &mut R,
+ ) -> Result<Self> {
+ let need_mutate = match rewriter.pre_visit(&self)? {
+ RewriteRecursion::Mutate => return rewriter.mutate(self),
+ RewriteRecursion::Stop => return Ok(self),
+ RewriteRecursion::Continue => true,
+ RewriteRecursion::Skip => false,
+ };
+
+ let after_op_children =
+ self.map_children(|node| node.transform_using(rewriter))?;
+
+ // now rewrite this node itself
+ if need_mutate {
+ rewriter.mutate(after_op_children)
+ } else {
+ Ok(after_op_children)
+ }
+ }
+
+ /// Convenience utils for writing optimizers rule: recursively apply the
given `op` to the node tree.
+ /// When `op` does not apply to a given node, it is left uncshanged.
+ /// The default tree traversal direction is transform_up(Postorder
Traversal).
+ fn transform<F>(self, op: &F) -> Result<Self>
+ where
+ F: Fn(Self) -> Option<Self>,
+ {
+ self.transform_up(op)
+ }
+
+ /// Convenience utils for writing optimizers rule: recursively apply the
given 'op' to the node and all of its
+ /// children(Preorder Traversal).
+ /// When the `op` does not apply to a given node, it is left unchanged.
+ fn transform_down<F>(self, op: &F) -> Result<Self>
+ where
+ F: Fn(Self) -> Option<Self>,
+ {
+ let node_cloned = self.clone();
+ let after_op = match op(node_cloned) {
+ Some(value) => value,
+ None => self,
+ };
+ after_op.map_children(|node| node.transform_down(op))
+ }
+
+ /// Convenience utils for writing optimizers rule: recursively apply the
given 'op' first to all of its
+ /// children and then itself(Postorder Traversal).
+ /// When the `op` does not apply to a given node, it is left unchanged.
+ fn transform_up<F>(self, op: &F) -> Result<Self>
+ where
+ F: Fn(Self) -> Option<Self>,
+ {
+ let after_op_children = self.map_children(|node|
node.transform_up(op))?;
+
+ let after_op_children_clone = after_op_children.clone();
+ let new_node = match op(after_op_children) {
+ Some(value) => value,
+ None => after_op_children_clone,
+ };
+ Ok(new_node)
+ }
+
+ /// Apply transform `F` to the node's children, the transform `F` might
have a direction(Preorder or Postorder)
+ fn map_children<F>(self, transform: F) -> Result<Self>
+ where
+ F: FnMut(Self) -> Result<Self>;
+}
+
+/// Trait for potentially recursively transform an [`TreeNodeRewritable`] node
+/// tree. When passed to `TreeNodeRewritable::transform_using`,
`TreeNodeRewriter::mutate` is
+/// invoked recursively on all nodes of a tree.
+pub trait TreeNodeRewriter<N: TreeNodeRewritable>: Sized {
+ /// Invoked before (Preorder) any children of `node` are rewritten /
+ /// visited. Default implementation returns
`Ok(RewriteRecursion::Continue)`
+ fn pre_visit(&mut self, _node: &N) -> Result<RewriteRecursion> {
+ Ok(RewriteRecursion::Continue)
+ }
+
+ /// Invoked after (Postorder) all children of `node` have been mutated and
+ /// returns a potentially modified node.
+ fn mutate(&mut self, node: N) -> Result<N>;
+}
+
+/// Controls how the [TreeNodeRewriter] recursion should proceed.
+#[allow(dead_code)]
+pub enum RewriteRecursion {
+ /// Continue rewrite / visit this node tree.
+ Continue,
+ /// Call 'op' immediately and return.
+ Mutate,
+ /// Do not rewrite / visit the children of this node.
+ Stop,
+ /// Keep recursive but skip apply op on this node
+ Skip,
+}
+
+impl TreeNodeRewritable for Arc<dyn PhysicalExpr> {
+ fn map_children<F>(self, transform: F) -> Result<Self>
+ where
+ F: FnMut(Self) -> Result<Self>,
+ {
+ if !self.children().is_empty() {
+ let new_children: Result<Vec<_>> =
+ self.children().into_iter().map(transform).collect();
+ with_new_children_if_necessary(self, new_children?)
+ } else {
+ Ok(self)
+ }
+ }
+}
+
+/// Returns a copy of this expr if we change any child according to the
pointer comparison.
+/// The size of `children` must be equal to the size of
`PhysicalExpr::children()`.
+/// Allow the vtable address comparisons for PhysicalExpr Trait Objects，it is
harmless even
+/// in the case of 'false-native'.
+#[allow(clippy::vtable_address_comparisons)]
+pub fn with_new_children_if_necessary(
+ expr: Arc<dyn PhysicalExpr>,
+ children: Vec<Arc<dyn PhysicalExpr>>,
+) -> Result<Arc<dyn PhysicalExpr>> {
+ if children.len() != expr.children().len() {
+ Err(DataFusionError::Internal(
+ "PhysicalExpr: Wrong number of children".to_string(),
+ ))
+ } else if children.is_empty()
+ || children
+ .iter()
+ .zip(expr.children().iter())
+ .any(|(c1, c2)| !Arc::ptr_eq(c1, c2))
+ {
+ expr.with_new_children(children)
+ } else {
+ Ok(expr)
+ }
+}
+
+/// Compare the two expr lists are equal no matter the order.
+/// For example two InListExpr can be considered to be equals no matter the
order:
+///
+/// In('a','b','c') == In('c','b','a')
+pub fn expr_list_eq_any_order(
Review Comment:
This seems like it could be in a utils or some other module
--
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]
