Skip to content

add short circuit in BinaryExpr #11247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
acking-you wants to merge 3 commits into from
Closed

add short circuit in BinaryExpr #11247

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fda9a6c
[draft] add shot circuit in BinaryExpr
acking-you Jul 3, 2024
9510783
refactor: add check_short_circuit function
acking-you Jul 3, 2024
6d92321
refactor: change if condition to match
acking-you Jul 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions datafusion/physical-expr/src/expressions/binary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,50 @@ impl PhysicalExpr for BinaryExpr {
fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
use arrow::compute::kernels::numeric::*;

fn check_short_circuit(arg: &ColumnarValue, op: &Operator) -> bool {
let data_type = arg.data_type();
match (data_type, op) {
(DataType::Boolean, Operator::And) => {
match arg {
ColumnarValue::Array(array) => {
if let Ok(array) = as_boolean_array(&array) {
return array.true_count() == 0;
Copy link
Copy Markdown
Contributor

@alamb alamb Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW this true_count operation requires checking all values (which should still be fast, but I wanted to point out it was not free / constant time): https://docs.rs/arrow-array/52.0.0/src/arrow_array/array/boolean_array.rs.html#142

Copy link
Copy Markdown
Contributor

@Dandandan Dandandan Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably can speed it up quite a bit by providing a function that doesn't count but provides whether the bitmap is all true / all false 🤔

}
}
ColumnarValue::Scalar(scalar) => {
if let ScalarValue::Boolean(Some(value)) = scalar {
return !value;
}
}
}
false
}
(DataType::Boolean, Operator::Or) => {
match arg {
ColumnarValue::Array(array) => {
if let Ok(array) = as_boolean_array(&array) {
return array.true_count() == array.len();
}
}
ColumnarValue::Scalar(scalar) => {
if let ScalarValue::Boolean(Some(value)) = scalar {
return *value;
}
}
}
false
}
_ => false,
}
}

let lhs = self.left.evaluate(batch)?;

// Optimize for short-circuiting `Operator::And` or `Operator::Or` operations and return early.
if check_short_circuit(&lhs, &self.op) {
return Ok(lhs);
}

let rhs = self.right.evaluate(batch)?;
let left_data_type = lhs.data_type();
let right_data_type = rhs.data_type();
Expand Down