alamb commented on code in PR #7732:
URL: https://github.com/apache/arrow-datafusion/pull/7732#discussion_r1353069618
##########
datafusion/expr/src/expr_rewriter/mod.rs:
##########
@@ -265,7 +265,6 @@ mod test {
use arrow::datatypes::DataType;
use datafusion_common::tree_node::{RewriteRecursion, TreeNode,
TreeNodeRewriter};
use datafusion_common::{DFField, DFSchema, ScalarValue};
- use std::ops::Add;
Review Comment:
Implementing `std::ops::Add` means you can write something like `expr1 +
expr2`.
##########
datafusion/expr/src/expr.rs:
##########
@@ -1032,6 +1033,88 @@ impl Expr {
}
}
+impl Expr {
+ // Operation methods from traits.
+
+ //
+ // Arithmetic operations
+ //
+
+ /// Return `self + other`
+ #[allow(clippy::should_implement_trait)]
Review Comment:
> user may require to include specific traits from std::ops::* in their
source.
I think using the `std::ops` in their traits is the less surprising API and
is the "idiomatic" rust way to do this. I think we should remove these methods
and just use `std::ops`
> Or, should we re-export these traits in prelude?
This would be ok with me. Alternately we could add an example to
https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.Expr.html saying
that it is possible to use the `std::ops` and show an example of `a + b` or the
equivalent
--
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]
