Fix bug where optimizer was removing Partitioning::DistributeBy expressions
#3229
Conversation
| .. | ||
| }) => match partitioning_scheme { | ||
| Partitioning::Hash(expr, _) => expr.clone(), | ||
| _ => vec![], |
There was a problem hiding this comment.
This catch-all is partly responsible for the bug creeping in, so I removed it.
There was a problem hiding this comment.
This bites me all the time in my code base. Good find! I ran this PR against the test I was seeing the issue in and it resolved my issue.
Codecov Report
@@ Coverage Diff @@
## master #3229 +/- ##
==========================================
- Coverage 85.83% 85.83% -0.01%
==========================================
Files 291 293 +2
Lines 52947 53162 +215
==========================================
+ Hits 45447 45629 +182
- Misses 7500 7533 +33
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
andygrove
changed the title
|
@jdye64 This is ready for review |
jdye64
left a comment
jdye64
left a comment
There was a problem hiding this comment.
Changes fixed my issue and really like the inclusion of the integration tests that run through the optimizer. I think in general that is a good thing to have. Thanks @andygrove !
| .. | ||
| }) => match partitioning_scheme { | ||
| Partitioning::Hash(expr, _) => expr.clone(), | ||
| _ => vec![], |
There was a problem hiding this comment.
This bites me all the time in my code base. Good find! I ran this PR against the test I was seeing the issue in and it resolved my issue.
andygrove
changed the title
Partitioning::DistributeBy expressions
|
Benchmark runs are scheduled for baseline = 74872a3 and contender = e1d4069. e1d4069 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
4 participants
Which issue does this PR close?
Closes #3234
Rationale for this change
Optimizer rules re-write
DistributeBypartitioning without any expressions without this fix.What changes are included in this PR?
Partitioning::DistributeByAre there any user-facing changes?