feat: optimize CASE WHEN for divide-by-zero protection pattern #19994
+231
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CuteChuanChuan
force-pushed
the
raymond/11570-optimize-case-when
branch
2 times, most recently
from
267d8df to
334785f
Compare
Contributor
|
run benchmark tpcds |
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
Hi @andygrove , |
Contributor
|
run benchmark tpcds |
Contributor
|
Also FYI @pepijnve |
|
🤖 |
Contributor
|
It might be useful to add a microbenchmark to https://github.com/apache/datafusion/blob/main/datafusion/physical-expr/benches/case_when.rs so we can compare before/after. I don't think any of the existing ones will cover this particular pattern. The TCP-DS queries that contain this pattern (based on |
|
🤖: Benchmark completed Details
|
- Adds a specialization for the common pattern: CASE WHEN y > 0 THEN x / y ELSE NULL END - Add EvalMethod::DivideByZeroProtection variant - Add pattern detection in find_best_eval_method() - Implement safe_divide using Arrow kernels - Handle CastExpr wrapping on divisor
CuteChuanChuan
force-pushed
the
raymond/11570-optimize-case-when
branch
from
334785f to
6479884
Compare
Contributor
Author
|
Microbenchmark result:
|
Contributor
Author
|
Hi @pepijnve , thanks for pointing out the need for a microbenchmark. I compared |
alamb
reviewed
Jan 29, 2026
| } | ||
| } | ||
|
|
||
| fn benchmark_divide_by_zero_protection(c: &mut Criterion, batch_size: usize) { |
Contributor
There was a problem hiding this comment.
Could you please pull this additional benchmark code code into its own PR we could then use our benchmarking scripts to compare performance of this PR with main?
Thank you 🙏
Contributor
Author
pepijnve
reviewed
Jan 30, 2026
| }, | ||
| ); | ||
|
|
||
| group.bench_function( |
Contributor
There was a problem hiding this comment.
I'm trying to spot the difference between this benchmark and the one just above it, they look identical to me. Yet the measurements seem to produce similar values. What causes that?
Contributor
Author
There was a problem hiding this comment.
Hi @pepijnve ,
The key difference is in which column is used in the WHEN condition:
- First benchmark (
DivideByZeroProtection): checks divisor_col > 0, and since divisor_col is also the divisor in numerator / divisor_col, this matches the pattern and triggers the optimization. - Second benchmark (
ExpressionOrExpression): checks divisor_copy_col > 0, but the division uses divisor_col. Since the checked column doesn't match the divisor, the optimization is not triggered and it falls back to ExpressionOrExpression.
I'll also add a comment in the code to make this distinction clearer.
Contributor
There was a problem hiding this comment.
Ah of course. That's really subtle. A comment for future readers is indeed a good idea.
Contributor
There was a problem hiding this comment.
A followup question I had, but I'm happy to measure that myself, is what's causing the performance gap? Might be interesting to see if there's something we could do to improve ExprOrExpr for this particular pattern.
Contributor
|
Benchmark was added in #20076 |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 30, 2026
## Which issue does this PR close? Related to #19994 - This PR extracts the benchmark code to allow performance comparison. ## Rationale for this change As pointed out by @alamb in #19994, this separates the microbenchmark code so that the benchmarking scripts can compare the optimization PR against main with the benchmark already in place. ## What changes are included in this PR? Adds a microbenchmark for the divide-by-zero protection pattern in `case_when.rs`: - Benchmarks with varying percentages of zeros (0%, 10%, 50%, 90%) - Compares `DivideByZeroProtection` pattern (where checked column matches divisor) vs `ExpressionOrExpression` fallback (where they don't match) ## Are these changes tested? benchmark code only. ## Are there any user-facing changes? No.
Contributor
|
I merged up to get the new benchmarks so I could run them on our runner |
Contributor
|
run benchmark case_when |
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
|
run benchmark case_when |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
Dandandan
approved these changes
Jan 31, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
The
CaseExprimplementation is expensive. A common usage pattern (particularly in TPC-DS benchmarks) is to protect against divide-by-zero:This entire expression can be replaced with a simpler divide operation that returns NULL when the divisor is zero, avoiding the overhead of full CASE evaluation.
What changes are included in this PR?
Are these changes tested?
Yes, added two new tests:
Are there any user-facing changes?
No. This is an internal optimization that produces the same results but with better performance.