blinding-pixels opened a new pull request, #23627: URL: https://github.com/apache/datafusion/pull/23627
## Which issue does this PR close? - Closes #23572 - Part of #22395 ## 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: - Exact, subset, or incompatible keys - Subset threshold met or not met - Preserve-file threshold met or not met - Target partition count equal to or greater than the input count 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: - Exact Range keys satisfy the requirement directly in `range_satisfies_key_partitioning`. - Subset keys require `allow_subset_satisfy_partitioning`. - Subset satisfaction is enabled when the subset threshold is met, or when the preserve threshold is met and increasing the target partition count is the only reason to repartition. - Incompatible keys never satisfy the requirement. This explains why these cases have different outcomes: - Exact key, threshold not met, target equal: reuse - Subset key, threshold not met, preserve not met, target equal: hash 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 `RangeSatisfactionConfigCase` structure and eight configuration rows: - Two subset-threshold states - Two preserve-threshold states - Two target-partition states Each row contains explicit expected results for exact, subset, and incompatible keys, producing all 24 combinations. For every combination, the test: 1. Creates the same four-partition Range input. 2. Applies the selected key relationship and optimizer configuration. 3. Runs the real physical optimizer. 4. Checks whether the optimized plan contains a hash repartition. The expected values remain explicit in the table rather than being calculated using a second implementation of the optimizer logic. I also added `KeyPartitioningRequirementExec` under 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: - An aggregate harness introduced aggregate-specific round-robin repartitioning. - A window harness worked but coupled the shared matrix to behavior already tested by window-specific cases. - `OutputRequirementExec` and the existing `MockReqExec` request ordinary keyed distributions but cannot enable the crate-private Range opt-in. ## Are these changes tested? Yes. The following checks pass: - The complete 24-combination configuration matrix - All related Range physical-optimizer tests - The complete `range_partitioning.slt` file - Formatting and diff checks - Targeted Clippy checks ### Matrix mutation testing The nine expectations not directly specified by the issue were mutation-tested against their decision branches: | Mutation location and edit | Result | |---|---| | `datafusion/physical-plan/src/distribution_requirements.rs:459`: changed `PartitioningSatisfaction::Exact` to `PartitioningSatisfaction::NotSatisfied` | All three derived exact-key cells failed, along with the issue-specified exact cases using the same rule | | `datafusion/physical-optimizer/src/ensure_requirements/enforce_distribution.rs:1305`: appended `&& false` to `allow_subset_satisfy_partitioning` | All five derived subset cells expecting reuse failed: the four threshold-met combinations and the preserve-met/greater-target combination | | `datafusion/physical-optimizer/src/ensure_requirements/enforce_distribution.rs:1303`: changed `current_partitions < target_partitions` to `current_partitions <= target_partitions` | Exactly one cell failed: subset key, threshold not met, preserve met, target equal | The 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`: | Mutated operator and location | SLT failures | |---|---| | `AggregateExec` — `datafusion/physical-plan/src/aggregates/mod.rs:1952` | 5 — `range_partitioning.slt:41`, `:101`, `:137`, `:218`, and `:551` | | `HashJoinExec` — `datafusion/physical-plan/src/joins/hash_join/exec.rs:1297` | 5 — `range_partitioning.slt:280`, `:476`, `:514`, `:551`, and `:607` | | `BoundedWindowAggExec` — `datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs:336` | 2 — `range_partitioning.slt:701` and `:821` | | `WindowAggExec` — `datafusion/physical-plan/src/windows/window_agg_exec.rs:244` | 1 — `range_partitioning.slt:730` | Each mutation introduced unexpected hash repartitions. Line 551 protects both `AggregateExec` and `HashJoinExec`. This confirms that the two test layers protect different failure points: - The neutral matrix protects the shared configuration decision. - The SLT cases protect each operator’s opt-in. 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::test` module. I did not introduce a public Range-policy API because the current opt-in is documented as a temporary bridge pending #23266. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
