-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add short circuit evaluation for AND and OR
#15462
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.
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
Changes from 13 commits
3e2a299
57d6645
b85807d
bf4e218
c41465e
f171227
ac3c918
a642394
39855d3
2205edb
d79a75a
575c3f3
8801063
ad1210e
e190119
f2c4caa
0ef29b1
6ea2502
f8f4d6f
13742d2
1394f3f
59cfced
775e70a
11816e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -359,6 +359,12 @@ impl PhysicalExpr for BinaryExpr { | |
| use arrow::compute::kernels::numeric::*; | ||
|
|
||
| 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(); | ||
|
|
@@ -805,6 +811,47 @@ impl BinaryExpr { | |
| } | ||
| } | ||
|
|
||
| /// Check if it meets the short-circuit condition | ||
| /// 1. For the `AND` operator, if the `lhs` result all are `false` | ||
| /// 2. For the `OR` operator, if the `lhs` result all are `true` | ||
| /// 3. Otherwise, it does not meet the short-circuit condition | ||
| 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.false_count() == array.len(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I remember this had some overhead (for calculating the counts) from a previous try.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think the overhead added here should be very small (the compiler optimization should work well), and the test results we discussed before were sometimes fast and sometimes slow (maybe noise). Your suggestion of making an early judgment and returning false seems like a good idea, but I'm not sure if it will be effective.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either way, we can use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thank you for your suggestion. I will try it later.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be overkill, but one could try a sampling approach: Run the loop with the early exit for the first few chunks, and then switch over to the unconditional loop. Almost seems like something the compiler could automagically do...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thank you for your suggestion, but if we're only applying conditional checks to the first few blocks, then I feel this optimization might not be meaningful. If nearly all blocks can be filtered out by the preceding filter, the optimization will no longer be effective.
I tend to agree with @alamb's point that if the overhead of verification is somewhat unacceptable, adopting some heuristic approaches would be better.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked more carefully at bool_or and I do think it would be faster than this implementation on the case where there are some true values (as it stops as soon as it finds a single non zero): https://docs.rs/arrow/latest/arrow/compute/fn.bool_or.html |
||
| } | ||
| } | ||
| 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, | ||
| } | ||
| } | ||
|
|
||
| fn concat_elements(left: Arc<dyn Array>, right: Arc<dyn Array>) -> Result<ArrayRef> { | ||
| Ok(match left.data_type() { | ||
| DataType::Utf8 => Arc::new(concat_elements_utf8( | ||
|
|
@@ -4832,4 +4879,39 @@ mod tests { | |
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_check_short_circuit() { | ||
| use crate::planner::logical2physical; | ||
| use datafusion_expr::col as logical_col; | ||
| use datafusion_expr::lit; | ||
| let schema = Arc::new(Schema::new(vec![ | ||
| Field::new("a", DataType::Int32, false), | ||
| Field::new("b", DataType::Int32, false), | ||
| ])); | ||
| let a_array = Int32Array::from(vec![1, 3, 4, 5, 6]); | ||
| let b_array = Int32Array::from(vec![1, 2, 3, 4, 5]); | ||
| let batch = RecordBatch::try_new( | ||
| Arc::clone(&schema), | ||
| vec![Arc::new(a_array), Arc::new(b_array)], | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| // op: AND left: all false | ||
| let left_expr = logical2physical(&logical_col("a").eq(lit(2)), &schema); | ||
| let left_value = left_expr.evaluate(&batch).unwrap(); | ||
| assert!(check_short_circuit(&left_value, &Operator::And)); | ||
| // op: AND left: not all false | ||
| let left_expr = logical2physical(&logical_col("a").eq(lit(3)), &schema); | ||
| let left_value = left_expr.evaluate(&batch).unwrap(); | ||
| assert!(!check_short_circuit(&left_value, &Operator::And)); | ||
| // op: OR left: all true | ||
| let left_expr = logical2physical(&logical_col("a").gt(lit(0)), &schema); | ||
| let left_value = left_expr.evaluate(&batch).unwrap(); | ||
| assert!(check_short_circuit(&left_value, &Operator::Or)); | ||
| // op: OR left: not all true | ||
| let left_expr = logical2physical(&logical_col("a").gt(lit(2)), &schema); | ||
| let left_value = left_expr.evaluate(&batch).unwrap(); | ||
| assert!(!check_short_circuit(&left_value, &Operator::Or)); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing some obvious points, but
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for your CR, your opinion is correct. I actually did the same. I'm very sorry. Maybe my comment is a little confusing. I will fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've optimized the comments, so it may look a bit clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I couldn't be clear in my last comment.
This PR short-circuits this case:
I have 2 further ideas to discuss:
1)
Why don't we check this case as well?
isn't this case optimizable as well? Are we handling those cases in another place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I understand now. However, the current execution order of BinaryExpr is fixed to evaluate the left side first and then the right side. The situation you mentioned I had also considered before, but wouldn't it be better to do it this way: the optimizer rewrites simpler expressions to the left as much as possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's imagine this case also (I adapt my second example such that homogenous side is lhs)
Doesn't this brings a clear gain by short-circuiting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand these now. Sorry, I didn't notice it just now and thought it was only a positional difference.
This is a great idea. I think we can open a new issue and add this optimization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you can rewrite expressions like this without changing the short-circuit semantics (if lHS is false then don't run the RHS in
a AND b)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need a rewrite. evaluate() needs to return a ColumnarValue, so we can just return the short-circuited versions in my examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about these?