Ted-Jiang commented on code in PR #5207:
URL: https://github.com/apache/arrow-datafusion/pull/5207#discussion_r1098567488
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -679,11 +679,11 @@ impl PhysicalExpr for BinaryExpr {
let scalar_result = match (&left_value, &right_value) {
(ColumnarValue::Array(array), ColumnarValue::Scalar(scalar)) => {
// if left is array and right is literal - use scalar
operations
- self.evaluate_array_scalar(array, scalar)?
+ self.evaluate_array_scalar(array, scalar.clone())?
Review Comment:
> Like rather than match(&left_value, &right_value) could the code be
match(left_value, right_value)
could not avoid clone scalar, because it has a fallback logic in
`evaluate_array_scalar ` and `evaluate_scalar_scalar `
like below 🤣
```rust
return match (left_value, right_value) {
(ColumnarValue::Array(left_array),
ColumnarValue::Scalar(scalar)) => {
// if left is array and right is literal - use scalar
operations
if let Some(result) =
self.evaluate_array_scalar(&left_array, scalar.clone())? {
result.map(|a| ColumnarValue::Array(a))
} else {
// if scalar operation is not supported - fallback to
array implementation
let right_array =
scalar.to_array_of_size(batch.num_rows());
self.evaluate_with_resolved_args(
left_array,
&left_data_type,
right_array,
&right_data_type,
)
.map(|a| ColumnarValue::Array(a))
}
}
(ColumnarValue::Scalar(scalar),
ColumnarValue::Array(right_array)) => {
// if right is literal and left is array - reverse operator
and parameters
if let Some(result) =
self.evaluate_scalar_array(scalar.clone(), &right_array)?
{
result.map(|a| ColumnarValue::Array(a))
} else {
// if scalar operation is not supported - fallback to
array implementation
let left_array =
scalar.to_array_of_size(batch.num_rows());
self.evaluate_with_resolved_args(
left_array,
&left_data_type,
right_array,
&right_data_type,
)
.map(|a| ColumnarValue::Array(a))
}
}
(ColumnarValue::Scalar(left_value),
ColumnarValue::Scalar(right_value)) => {
// Notice even both side are scalar also need cast to array
with batch size.
let (left, right) = (
left_value.to_array_of_size(batch.num_rows()),
right_value.to_array_of_size(batch.num_rows()),
);
self.evaluate_with_resolved_args(
left,
&left_data_type,
right,
&right_data_type,
)
.map(|a| ColumnarValue::Array(a))
}
(ColumnarValue::Array(left_array),
ColumnarValue::Array(right_array)) => self
.evaluate_with_resolved_args(
left_array,
&left_data_type,
right_array,
&right_data_type,
)
.map(|a| ColumnarValue::Array(a)),
};
```
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
