blinding-pixels commented on PR #23627:
URL: https://github.com/apache/datafusion/pull/23627#issuecomment-5006248110
Thanks for the review! I checked the RoundRobin concern and the proposed SLT
deletions together.
I agree that a `Reuse` expectation should reject any repartition. Though,
checking only for “any repartition” would allow a `Hash` case to pass with
RoundRobin. I therefore tested an assertion split by expected outcome:
- `Reuse` requires zero `RepartitionExec` nodes.
- `Hash` requires exactly one `RepartitionExec`, and it must be Hash.
I ran this assertion across all 24 matrix cells at
`datafusion/core/tests/physical_optimizer/enforce_distribution.rs:803`; every
cell passed.
```rust
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 `enforce_distribution.rs:890–898`
only checks whether Hash appears. A future neutral plan containing RoundRobin
could therefore pass a `Reuse` row because Hash is absent, while a plan
containing both Hash and RoundRobin could pass a `Hash` row because Hash is
present. The existing SLT plan comparisons would fail loudly in either
situation.
It is also worth making the two test layers explicit.
`KeyPartitioningRequirementExec` enables Range satisfaction itself at
`datafusion/physical-plan/src/test/exec.rs:331–335`. Therefore, changing or
removing a production operator’s opt-in cannot affect the 24 neutral matrix
cells. The matrix tests the shared decision after an operator has opted in; the
per-operator SLTs demonstrably test production operators using that decision.
At least one positive Range-reuse case must remain for every production
operator, with additional cases retained where operator-specific branches
require them.
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: `preserve_partial_aggregate_partitioning` is an input to the later
`add_roundrobin` decision.
| Production mutation | Exact mutation location | Failures within
`range_partitioning.slt` |
|---|---|---|
| Force aggregate Range preservation to `false` | Expression at
`datafusion/physical-optimizer/src/ensure_requirements/enforce_distribution.rs:1309–1315`;
result forced false at `:1315` | Test 7 only, query at
`range_partitioning.slt:218` |
| Force the join preserve shortcut to `false` | Expression at
`enforce_distribution.rs:1356–1360`; result forced false at `:1360` | Test 14
only, query at `range_partitioning.slt:476` |
| Remove the partitioned-join guard | Removed `&& !is_partitioned_join` at
`enforce_distribution.rs:1304` | Test 13 only, query at
`range_partitioning.slt:440` |
| Force RoundRobin insertion to `false` | `add_roundrobin` expression at
`enforce_distribution.rs:1317–1323`; result forced false at `:1323` | Tests 6
and 8, queries at `range_partitioning.slt:186` and `:247` |
### Safe to delete
| Test | Current location | Evidence |
|---|---|---|
| Test 4 | Heading `range_partitioning.slt:122`; query `:137` | Its
exact-key, threshold-not-met, equal-target configuration is covered by the
matrix row at
`datafusion/core/tests/physical_optimizer/enforce_distribution.rs:813–818`.
Aggregate opt-in removal would remain caught by Tests 1, 3, and 16 at SLT lines
`41`, `101`, and `551`, independently of Test 7. |
| Test 5 | Heading `range_partitioning.slt:146`; query `:161` | Its
subset-key, threshold-not-met, equal-target configuration is covered by the
same matrix row at `enforce_distribution.rs:813–818`. |
| Test 6 | Heading `range_partitioning.slt:172`; query `:186` | Its shared
Hash decision is covered by the matrix row at
`enforce_distribution.rs:819–824`. Tests 6 and 8 both failed when RoundRobin
insertion was disabled, so either one can carry the aggregate-specific
RoundRobin coverage. I propose retaining Test 8 because it pairs directly with
Test 7’s preserve-threshold-met case. |
### Retain under the proposed trim
| Test | Current location | Evidence |
|---|---|---|
| Test 7 | Heading `range_partitioning.slt:204`; query `:218` | It was the
only failure in this SLT file when `preserve_partial_aggregate_partitioning`
was forced false at `enforce_distribution.rs:1315`. |
| Test 8 | Heading `range_partitioning.slt:233`; query `:247` | Either Test
6 or Test 8 must remain to detect loss of aggregate RoundRobin insertion. Under
the proposed deletion of Test 6, Test 8 carries that coverage and forms the
threshold-not-met counterpart to Test 7. |
| Test 13 | Heading `range_partitioning.slt:431`; query `:440` | It was the
only failure in this SLT file when the partitioned-join guard at
`enforce_distribution.rs:1304` was removed. |
| Test 14 | Heading `range_partitioning.slt:468`; query `:476` | It was the
only failure in this SLT file when `preserve_satisfying_file_partitioning` was
forced false at `enforce_distribution.rs:1360`. |
The proposed sequence is therefore:
1. Replace the Hash-only assertion with the verified outcome-specific
assertion.
2. Delete Tests 4, 5, and 6.
3. Retain Tests 7, 8, 13, and 14 because the mutations show they protect
production-operator behavior the neutral matrix does not exercise.
If this sounds like it is in the right direction, I can make these and push.
--
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]
