feat: complete range repartition physical planning#23617
Conversation
|
cc @gene-bordegaray & @gabotechs ! Thanks in advance for any feedback! 😁 🎉 |
gene-bordegaray
left a comment
There was a problem hiding this comment.
left some feedback, thank you 🙇
|
@gene-bordegaray thanks for the feedback! Pushed a commit to address - appreciate all the time taken reviewing! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23617 +/- ##
========================================
Coverage 80.65% 80.65%
========================================
Files 1086 1086
Lines 366521 366814 +293
Branches 366521 366814 +293
========================================
+ Hits 295618 295858 +240
- Misses 53274 53296 +22
- Partials 17629 17660 +31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gene-bordegaray
left a comment
There was a problem hiding this comment.
Thanks for updating this, I think my earlier comment may have pushed this a little too far in the helper direction 😅
What I was trying to get at was mostly reducing the repeated range-partitioning setup, not adding a bunch of extra test scaffolding / coverage. I think we can keep this much tighter:
- one positive projection-swap test that verifies the range key is rewritten when its projected column index changes
- one negative projection-swap test that covers dropping a single key and dropping part of a compound key
- two sort-pushdown tests: supported when the range repartition maintains input order, unsupported when it does not
- keep the repartitioned() None test
I’d drop the extra execution sortedness test since this PR really about the planning behavior.
For helpers I think the useful ones are only the small range-specific setup things like build range partitioning from cols, build a projection from cols, unwrap/assert range partitioning.
Which issue does this PR close?
Rationale for this change
After #23231 was merged in for supporting physical execution of the range repartitioning scheme, we still had a few methods on physical planning unimplemented, specifically
try_swapping_with_projection,try_pushdown_sort,repartitioned- this PR finishes the implementation of those methodsWhat changes are included in this PR?
try_swapping_with_projection: similar to theHashscheme, forRangewe callupdate_exprfor each of the range key expressions to attempt rewriting based on the projection expressionstry_pushdown_sort: same as other variants, we delegate to the child and wrap with a newRepartitionExecrepartitioned: unable to support for Range, left comment in codebase with explanationAre these changes tested?
Yes
Are there any user-facing changes?
No