allow interleaveExec to support Range partioning#23623
Conversation
ddf65fb to
8242bfa
Compare
gene-bordegaray
left a comment
There was a problem hiding this comment.
some comments on tests but overall looks good
could we also add smoke test in enforce_distribution.rs for smoke tests (see other related PR)s
thank you 🙇
| } | ||
|
|
||
| #[test] | ||
| fn test_can_interleave_range_matching() -> Result<()> { |
There was a problem hiding this comment.
what do you think about making this a general distribution since that seems to be lacking in the file. A matrixs test case like:
- matching hash
- matching range
- subset hash
- subset range
then equivalently below we can have the ngative test for distributions that interleave shouldnt accept:
- mixing hash and range
- incompatible hashes
- incompatible ranges
- sort and null order variants for ranges
- etc
just a thought lmk
There was a problem hiding this comment.
This sounds great. Ill update the PR
| ---- | ||
| physical_plan | ||
| 01)UnionExec | ||
| 01)InterleaveExec |
There was a problem hiding this comment.
could we also add:
- a sub set test
- negative test (fallback to union)
- union into another operator test to ensure propagation
@gene-bordegaray are you referring to a case like this shouldn't be accepted right? Interleave should fall back to unionExec in this case. |
@Rich-T-kid yes exactly 🙇 |
|
@gene-bordegaray this is ready for review now |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23623 +/- ##
=======================================
Coverage ? 80.65%
=======================================
Files ? 1086
Lines ? 366165
Branches ? 366165
=======================================
Hits ? 295317
Misses ? 53236
Partials ? 17612 ☔ 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.
apprved once last things addressed. thakn you 👍
cc: @gabotechs
|
updated the PR to address your comments. |
e2b0786 to
68ea81a
Compare
|
Thanks @Rich-T-kid and @gene-bordegaray, this looks good to me. |
68ea81a to
f6f7a64
Compare
Which issue does this PR close?
InterleaveExecto preservePartitioning::Range#23455.Rationale for this change
see #23455
What changes are included in this PR?
The goal of this PR is to allow range partitioning to propagate through
InterleaveExec. Updatedcan_interleave()to acceptPartitioning::Rangewhen all children share an identical RangePartitioning (same ordering and split points), matching the existing behavior forPartitioning::Hash.Updated
range_partitioning.sltto expect InterleaveExec where it previously expected UnionExec, sincecan_interleave()now accepts Partitioning::Range.Are these changes tested?
yes, the
range_partioning.sltfile as well as theunion.sltfile sql logic test. This PR also includes three test.Are there any user-facing changes?
physical plans may look different now.