alamb commented on code in PR #9304:
URL: https://github.com/apache/arrow-datafusion/pull/9304#discussion_r1509959592
##########
datafusion/expr/src/udf.rs:
##########
@@ -338,6 +346,13 @@ pub trait ScalarUDFImpl: Debug + Send + Sync {
fn monotonicity(&self) -> Result<Option<FuncMonotonicity>> {
Ok(None)
}
+
+ // Do the function rewrite.
Review Comment:
```suggestion
/// Optionally apply per-UDF simplification / rewrite rules
///
/// This can be used to apply function specific simplification rules
/// during optimization (e.g. `arrow_cast` --> `Expr::Cast`).
///
/// Note that since DataFusion handles simplifying arguments
/// as well as "constant folding" (replacing a function call with
constant
/// arguments such as `my_add(1,2) --> 3` ) there is no need to
implement such
/// optimizations for UDFs.
```
##########
datafusion/wasmtest/src/lib.rs:
##########
@@ -17,9 +17,10 @@
extern crate wasm_bindgen;
use datafusion_common::{DFSchema, ScalarValue};
+use datafusion_expr::execution_props::ExecutionProps;
Review Comment:
Would it be possible to leave backwards compatibility `pub use` statements
to avoid a breaking API change?
Specifically I am thinking of users who had
```
use datafusion_optimizer::simplify_expressions::{ExprSimplifier,
SimplifyContext};
```
If we put this in `datafusion_optimizer::simplify_expressions` I think
existing code wouldn't need a change
```rust
// backwards compatibility
pub use datafusion_expr::simplify::SimplifyContext;
pub use datafusion_optimizer::simplify_expressions::ExprSimplifier;
```
Also in `datafusion_physical_expr` something like
```rust
// backards compatbolituy
pub mod execution_props {
pub use datafusion_expr::execution_props::ExecutionProps;
}
```
##########
datafusion/expr/src/simplify.rs:
##########
@@ -41,40 +41,12 @@ pub trait SimplifyInfo {
/// Returns data type of this expr needed for determining optimized int
type of a value
fn get_data_type(&self, expr: &Expr) -> Result<DataType>;
+
+ /// Return the schema for function simplifier
Review Comment:
If we are going to move the SimplifyInfo, I think we should now also remove
the schema requirement from this API
##########
datafusion/wasmtest/src/lib.rs:
##########
@@ -17,9 +17,10 @@
extern crate wasm_bindgen;
use datafusion_common::{DFSchema, ScalarValue};
+use datafusion_expr::execution_props::ExecutionProps;
Review Comment:
Here is an example:
https://github.com/apache/arrow-datafusion/blob/a1ae15826245097e7c12d4f0ed3425b25af6c431/datafusion/core/src/datasource/mod.rs#L38-L39
##########
datafusion/optimizer/src/simplify_expressions/function_simplifier.rs:
##########
@@ -0,0 +1,54 @@
+// 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.
+
+//! This module implements a rule that do function simplification.
+
+use datafusion_common::tree_node::TreeNodeRewriter;
+use datafusion_common::Result;
+use datafusion_expr::simplify::Simplified;
+use datafusion_expr::simplify::SimplifyInfo;
+use datafusion_expr::{expr::ScalarFunction, Expr, ScalarFunctionDefinition};
+
+pub(super) struct FunctionSimplifier<'a, S> {
Review Comment:
I think this would be more discoverable (and avoid another tree walk) if the
code was inlined into the existing ExprSimplifier (the giant `match` statement
that handles all the different kinds of `Expr`s) -- perhaps right after the
other rewrites for `Expr::ScalarFunction` ?
--
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]
