Add config-matrix tests in enforce_distribution.rs for range-satisfaction settings#23627
Draft
blinding-pixels wants to merge 1 commit into
Draft
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 existing SQL logic tests exercise Range reuse through aggregates, joins, and windows. Repeating the same configuration permutations for every operator would make those tests large and difficult to maintain.
Issue #23572 describes six wildcard rules across four dimensions:
Those rules directly specify 15 of the 24 possible combinations. The remaining nine expectations are derived from the documented semantics rather than recorded from optimizer output:
range_satisfies_key_partitioning.allow_subset_satisfy_partitioning.This explains why these cases have different outcomes:
The equal partition count is not a global reuse shortcut. The input must first satisfy the distribution requirement. Exact keys satisfy directly, while subset keys require permission.
What changes are included in this PR?
The matrix uses one
RangeSatisfactionConfigCasestructure and eight configuration rows:Each row contains explicit expected results for exact, subset, and incompatible keys, producing all 24 combinations.
For every combination, the test:
The expected values remain explicit in the table rather than being calculated using a second implementation of the optimizer logic.
I also added
KeyPartitioningRequirementExecunder the existing physical-plan test utilities. It is a neutral single-input operator that requests keyed partitioning and enables the existing Range opt-in. This lets the matrix test the shared decision without aggregate, join, or window behavior affecting the result.I considered several test harnesses before using the neutral operator:
OutputRequirementExecand the existingMockReqExecrequest ordinary keyed distributions but cannot enable the crate-private Range opt-in.Are these changes tested?
Yes.
The following checks pass:
range_partitioning.sltfileMatrix mutation testing
The nine expectations not directly specified by the issue were mutation-tested against their decision branches:
datafusion/physical-plan/src/distribution_requirements.rs:459: changedPartitioningSatisfaction::ExacttoPartitioningSatisfaction::NotSatisfieddatafusion/physical-optimizer/src/ensure_requirements/enforce_distribution.rs:1305: appended&& falsetoallow_subset_satisfy_partitioningdatafusion/physical-optimizer/src/ensure_requirements/enforce_distribution.rs:1303: changedcurrent_partitions < target_partitionstocurrent_partitions <= target_partitionsThe final mutation changed one character and flipped only one matrix cell. This shows that the matrix distinguishes the preserve/target boundary rather than several cells merely depending on the same broad condition.
All mutations were restored, and the complete matrix passed again.
Operator opt-in mutation testing
I also independently removed each production Range opt-in and ran
range_partitioning.slt:AggregateExec—datafusion/physical-plan/src/aggregates/mod.rs:1952range_partitioning.slt:41,:101,:137,:218, and:551HashJoinExec—datafusion/physical-plan/src/joins/hash_join/exec.rs:1297range_partitioning.slt:280,:476,:514,:551, and:607BoundedWindowAggExec—datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs:336range_partitioning.slt:701and:821WindowAggExec—datafusion/physical-plan/src/windows/window_agg_exec.rs:244range_partitioning.slt:730Each mutation introduced unexpected hash repartitions. Line 551 protects both
AggregateExecandHashJoinExec.This confirms that the two test layers protect different failure points:
When the SLT permutations are slimmed, the load-bearing operator cases above should remain. I would appreciate guidance on whether they should be marked as contract tests in place or moved into a dedicated section or file.
Are there any user-facing changes?
No query-planning or runtime behavior changes are included.
The only new exposed type is a test utility under the existing
datafusion_physical_plan::testmodule. I did not introduce a public Range-policy API because the current opt-in is documented as a temporary bridge pending #23266.