Add config-matrix tests in enforce_distribution.rs for range-satisfaction settings - #23627
Add config-matrix tests in enforce_distribution.rs for range-satisfaction settings#23627blinding-pixels wants to merge 4 commits into
Conversation
github-actions
Bot
added
core
|
Hi @gabotechs, this is my first contribution to DataFusion. Would you be able to trigger CI and review this PR when you have a chance? I’m happy to explain any of the decisions I made here. I’m looking forward to being a useful member of the community. Thank you! |
gene-bordegaray
left a comment
gene-bordegaray
left a comment
There was a problem hiding this comment.
overall looks good, thanks for banging this one out, this is very very much appreaciated 🙇
One goal was to keep range_partitioning.slt a bit leaner. I think we can do some of that now by eliminating some repetitive tests. I was thnking:
- TEST 4: Exact Range Aggregate Below Subset Threshold
- TEST 5: Range Subset Aggregate Rehashes Below Subset Threshold
- TEST 6: Aggregate Rehashes Below Subset Threshold
- TEST 7: Aggregate Preserves Range When Preserve File Threshold Met
- TEST 8: Aggregate Rehashes When Preserve File Threshold Not Met
- TEST 13: Compatible Range Join Repartitions to Increase Parallelism
- TEST 14: Preserve File Partitions Preserves Range Join Inputs
cc: @gabotechs let me know what you think. This reduces the noise in that file to focus on range end to end and this unit now covers all the tricky "above this" "below that" distribution stuff 👍
| let plan = config.to_plan(requirement, &DISTRIB_DISTRIB_SORT); | ||
| let plan = displayable(plan.as_ref()).indent(true).to_string(); | ||
| let has_hash_repartition = | ||
| plan.contains("RepartitionExec: partitioning=Hash"); |
There was a problem hiding this comment.
I think we should be checking for any repartition. For example we can have a RoundRobin inserted if we choose to increase parallelism when less than target_partitions and threshld values
|
Thanks for the review! I checked the RoundRobin concern and the proposed SLT deletions together. I agree that a
I ran this assertion across all 24 matrix cells at let repartitions = plan
.lines()
.filter(|line| line.contains("RepartitionExec:"))
.collect::<Vec<_>>();
match expected_plan {
ExpectedPlan::Reuse => {
assert!(repartitions.is_empty(), "{plan}");
}
ExpectedPlan::Hash => {
assert_eq!(repartitions.len(), 1, "{plan}");
assert!(
repartitions[0].contains("partitioning=Hash"),
"{plan}"
);
}
}This assertion is a prerequisite for relying on the matrix after trimming configuration SLTs. The existing assertion at It is also worth making the two test layers explicit. I mutation-tested the production branches associated with the proposed deletions. The locations below distinguish the complete expression from the specific condition changed. The first and fourth mutations intentionally overlap:
Safe to delete
Retain under the proposed trim
The proposed sequence is therefore:
If this sounds like it is in the right direction, I can make these and push. |
|
Hi @blinding-pixels, thanks for the PR! in order to be respectful with the time of the reviewers, please try to be concise in the PR description and the comments. If you are using an LLM for auto-responding feedback, please curate the answer before publishing it so that readers invest a reasonable amount of time digesting the information. |
|
Apologies, my bad on the length. Here is the short version- I agree on the assertion, split it by expected outcome so a Hash row can't pass with a RoundRobin. Reuse asserts zero On the trim, mutations say 4/5/6 are safe but 7, 13, and 14 are each the only test catching a specific branch. A reason I hesitated on the trim is that the neutral matrix enables the Range opt-in itself, so it stays green no matter what a production operator does. Only the SLTs catch that. Happy to push that and paste the mutation detail if you want it. The previous comment wasn't auto-generated but I did let an LLM write it up and didn't cut it down before posting. |
@blinding-pixels I am confused what you mea by brnach specific here. The tests I am sugesting to remove are in |
|
I see how my wording was confusing. The matrix uses a neutral synthetic operator, so it's a shared-contract test, not a universal operator contract test. It proves that once an operator enables Range satisfaction, the shared optimizer handles all 24 matrix combinations correctly. It does not prove that production aggregates, joins, and windows enable Range satisfaction or reach that shared logic correctly. The mutations show this:
To make the SLT file leaner, I could keep the neutral matrix for the shared contract and add a second Rust table using real aggregate and join plans. That would replace the optimizer-plan coverage of all the proposed SLT cases and allow them to be removed. The existing window SLTs would still protect the window operators. This would add roughly 80–100 lines to the existing Rust test file. I'm new to this codebase, so I'll defer to you on whether the added Rust coverage is worthwhile or you'd rather just drop the SLT cases. |
@blinding-pixels this makes sense. Can we also add a disabled option for the preserve file partition in the matrix 👍 |
|
Yes, for sure. I can add that. Including disabled as a third |
|
just the dabled and we can leave the SLTs and no focused aggregation or join case |
|
@gene-bordegaray I’ve added the requested rows for the disabled state. Please let me know if anything else needs to be added or removed! |
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23627 +/- ##
=======================================
Coverage ? 80.67%
=======================================
Files ? 1088
Lines ? 367640
Branches ? 367640
=======================================
Hits ? 296589
Misses ? 53391
Partials ? 17660 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@gene-bordegaray Is there anything else needed on this? Let me know. Thanks! |
|
I am going to be very busy for the next week, will get back to this thank you 🙇 |
gene-bordegaray
left a comment
gene-bordegaray
left a comment
There was a problem hiding this comment.
can we also please remove slt test 4 5 and 6
| } | ||
|
|
||
| #[derive(Debug, Clone, Copy)] | ||
| enum RangeKeyMatch { |
There was a problem hiding this comment.
Could we collapse all this to a small case structure using something like subset_met: bool, preserve_met: Option, and increase_partitions: bool, keeping the [ExpectedPlan; 3] table?
|
@gene-bordegaray I removed the three SLT tests and implemented your simplification suggestion. On a personal note, thank you for that part of the review. It helped me rethink how I structure test cases and pointed me in a better direction. |
gene-bordegaray
left a comment
gene-bordegaray
left a comment
There was a problem hiding this comment.
ok we are starting to get these now notice that we can reuse arts of test_utils.rs rather than repeating boilerplate and we can further simplify the test setup rather than being overly verbose (a test usually shouldn't be hundres of lines long).
Here is what I think should be done to make this test shorter and just as effective for coverage:
- Rebase onto the most recent
main - We can remove
KeyPartitioningRequirementExecand everything indatafusion/physical-plan/src/test/exec.rs - Extend the existing
RequirementsTestExecinphysical_optimizer/test_utils.rswith arequired_input_distribution: Distributionfield,with_required_input_distribution(...),input_distribution_requirements(). - Make the matrix simpler by doing:
let requirement = RequirementsTestExec::new(input) .with_required_input_distribution(Distribution::KeyPartitioned( partition_keys, )) .into_arc();
- Keep the matrix outcomes but do it as a simply:
const INPUT_PARTITIONS: usize = 4; const MET: usize = INPUT_PARTITIONS; const NOT_MET: usize = INPUT_PARTITIONS + 1; const DISABLED: usize = 0; const EQUAL: usize = INPUT_PARTITIONS; const GREATER: usize = INPUT_PARTITIONS + 1; // (subset, preserve, target, [exact, subset, incompatible]) let config_cases = [ (NOT_MET, DISABLED, EQUAL, [Reuse, Hash, Hash]), (NOT_MET, DISABLED, GREATER, [Hash, Hash, Hash]), ... ];
- Renumber SLT test after deleting 4, 5, and 6
…itions Remove redundant test setup and assertions without changing coverage.
593c96f to
37aeec3
Compare
|
@gene-bordegaray Done. It was nice being able to reuse |
4 participants
Which issue does this PR close?
Rationale for this change
Range partitioning reuse is controlled by shared optimizer settings, but the same configuration combinations were repeated across operator-specific SQL logic tests. This PR moves that shared coverage into one table-driven Rust test while retaining the operator-specific cases that test separate planning behavior.
What changes are included in this PR?
RequirementsTestExecwith a configurable input distribution.Are these changes tested?
Yes. The 36-cell matrix and
range_partitioning.sltpass.Are there any user-facing changes?
No. These changes only affect tests and test utilities.