jayzhan211 commented on PR #24766:
URL: https://github.com/apache/datafusion/pull/24766#issuecomment-5682601631
@goutamadwant,
This changes planning policy beyond the feature. When `scale()` returns
`InsufficientSamples`, the child is now left at its current partition count
instead of being key-repartitioned to `target_partitions` as on main. That
applies to every key requirement, not only co-partitioned ones, so e.g. an
aggregate over a 3-way exact range source now runs 3-way on a 16-core machine
(`range_requirement_scales_within_sample_resolution` pins target 7 -> 3
partitions with no exchange).
We already have a knob for "keep a satisfying layout instead of increasing
parallelism": `preserve_file_partitions` /
`preserve_satisfying_file_partitioning` a few lines above. This change makes
the disabled setting a no-op for range sources, which is why
`range_satisfaction_config_matrix` had to flip `(NOT_MET, DISABLED, GREATER)`
from `[Hash, Hash, Hash]` to `[Reuse, Hash, Hash]` and slt TEST 13 lost both
exchanges and its 5-way parallelism.
It can also be strictly worse than main for joins. In
`range_preservation_prefers_larger_unscalable_input` the larger side is a 3-way
exact range and the other side is already a native 4-way range at target. main:
one Range(4) exchange, 4-way join. This PR: one Range(3) exchange, 3-way join.
Same exchange count, less parallelism. The `.or_else` tie-break only covers the
unknown-size tie, not the strictly-larger case.
Suggest keeping main's behaviour on `InsufficientSamples` and dropping
`preserved_unscalable_range` plus the tie-break:
```rust
-
Err(RangePartitioningScaleError::InsufficientSamples { .. }) => {
- preserved_unscalable_range = true;
- None
- }
+ // Samples cannot support the target
parallelism:
+ // fall back to key repartitioning as
before.
+
Err(RangePartitioningScaleError::InsufficientSamples { .. }) => Some(
+ requirement
+ .clone()
+
.create_partitioning(target_partitions),
+ ),
```
and restoring the matrix / TEST 13 expectations. If retaining an unscalable
range for the co-partitioning pass is desirable, it should be gated on the
existing preserve_file_partitions threshold rather than unconditional, and the
reference selection must never let a preserved range beat a candidate that
already provides target_partitions.
--
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]
