findepi commented on code in PR #17554:
URL: https://github.com/apache/datafusion/pull/17554#discussion_r2348814471
##########
datafusion/physical-expr/src/intervals/test_utils.rs:
##########
@@ -22,9 +22,21 @@ use std::sync::Arc;
use crate::expressions::{binary, BinaryExpr, Literal};
use crate::PhysicalExpr;
use arrow::datatypes::Schema;
-use datafusion_common::{DataFusionError, ScalarValue};
+use datafusion_common::{DataFusionError, Result, ScalarValue};
+use datafusion_expr::execution_props::ExecutionProps;
use datafusion_expr::Operator;
+/// Helper function for tests that provides default ExecutionProps for binary
function calls
+fn binary_test(
Review Comment:
```suggestion
fn binary_expr(
```
##########
datafusion/physical-expr/src/intervals/test_utils.rs:
##########
@@ -22,9 +22,21 @@ use std::sync::Arc;
use crate::expressions::{binary, BinaryExpr, Literal};
use crate::PhysicalExpr;
use arrow::datatypes::Schema;
-use datafusion_common::{DataFusionError, ScalarValue};
+use datafusion_common::{DataFusionError, Result, ScalarValue};
+use datafusion_expr::execution_props::ExecutionProps;
use datafusion_expr::Operator;
+/// Helper function for tests that provides default ExecutionProps for binary
function calls
+fn binary_test(
+ lhs: Arc<dyn PhysicalExpr>,
+ op: Operator,
+ rhs: Arc<dyn PhysicalExpr>,
+ schema: &Schema,
+) -> Result<Arc<dyn PhysicalExpr>> {
+ let execution_props = ExecutionProps::new();
Review Comment:
make sure that default behavior configured by `ExecutionProps::new()` is to
fail on overflow
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -1095,8 +1096,14 @@ pub fn binary(
op: Operator,
rhs: Arc<dyn PhysicalExpr>,
_input_schema: &Schema,
+ execution_props: &ExecutionProps,
) -> Result<Arc<dyn PhysicalExpr>> {
- Ok(Arc::new(BinaryExpr::new(lhs, op, rhs)))
+ let fail_on_overflow = execution_props
+ .config_options
+ .as_ref()
+ .map(|cfg| cfg.execution.fail_on_overflow)
+ .unwrap_or(true);
+ Ok(Arc::new(BinaryExpr::new(lhs, op,
rhs).with_fail_on_overflow(fail_on_overflow)))
Review Comment:
Let's deprecate `BinaryExpr::new` and make sure all usages within DataFusion
codebase are updated correctly.
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -1141,7 +1148,18 @@ mod tests {
let left_expr = try_cast(left, schema, lhs)?;
let right_expr = try_cast(right, schema, rhs)?;
- binary(left_expr, op, right_expr, schema)
+ binary_test(left_expr, op, right_expr, schema)
+ }
+
+ /// Helper function for tests that creates a binary expression with
default ExecutionProps
+ fn binary_test(
Review Comment:
binary_test is suitable for a boolean-returning binary operators, e.g. `<`
or `=`
it's not suitable for others, e.g. `+`, `/`
```suggestion
fn binary_expr(
```
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -609,9 +609,21 @@ mod tests {
use datafusion_common::plan_err;
use datafusion_common::tree_node::{Transformed, TransformedResult,
TreeNode};
use datafusion_expr::type_coercion::binary::comparison_coercion;
+ use datafusion_expr::execution_props::ExecutionProps;
use datafusion_expr::Operator;
use datafusion_physical_expr_common::physical_expr::fmt_sql;
+ /// Helper function for tests that creates a binary expression with
default ExecutionProps
+ fn binary_test(
Review Comment:
```suggestion
fn binary_expr(
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
