andygrove opened a new issue, #2306:
URL: https://github.com/apache/datafusion-ballista/issues/2306
## Describe the bug
`ShuffleWriterExec`'s `DisplayAs` writes a string literal rather than a
value (`ballista/core/src/execution_plans/shuffle_writer.rs:624`):
```rust
// "None" is retained for plan-shape stability: this writer never
// repartitions, so the value can only ever be None.
DisplayFormatType::Default | DisplayFormatType::Verbose => {
write!(f, "ShuffleWriterExec: partitioning: None")
}
```
It used to print a value. The writer carried a `shuffle_output_partitioning:
Option<Partitioning>` field, and #2106 ("Remove the hash-partitioning shuffle
writer") deleted it, leaving `shuffle_output_partitioning()` as a method that
returns `None` unconditionally (line 803). Rather than drop the token, that PR
froze it as a literal so the 22 approved TPC-H plan-stability goldens would not
churn in the same diff. That was a reasonable expedient at the time, but the
comment reads as a permanent justification rather than a follow-up, and it has
stayed frozen since.
Three problems with the output as it stands:
1. **It carries no information, and is kept because it carries none.** A
constant cannot drift, so it cannot break a golden file. That is the golden
files driving the display rather than the other way round.
2. **It is misleading.** "partitioning: None" reads as "no output
partitioning", which is not true. The writer preserves its input partitioning,
and `try_new` sets its own `PlanProperties` to exactly the child's output
partitioning (line 442). The `None` refers to the absence of a *repartitioning
scheme*, which is an internal distinction a plan reader has no way to infer.
3. **The useful value is available and not shown.**
`self.properties().output_partitioning()` is the real partitioning, including
the partition count, and it renders exactly the way `SortShuffleWriterExec`
already renders its own (`partitioning=Hash([c_custkey@0], 16)`).
Point 3 has a concrete downstream cost. In #2305 the DOT graph was changed
to label nodes from each operator's `DisplayAs`, which works for every operator
except this one: because the text is a constant, that PR has to keep a
hand-written special case for `ShuffleWriterExec` just to recover the partition
count. A display impl that forces consumers to special-case around it is not
doing its job.
## To Reproduce
Every approved golden shows it, two or three times per query:
```shell
grep -c "ShuffleWriterExec: partitioning: None"
ballista/scheduler/tests/tpch_plan_stability/approved/q1.txt
```
```
ShuffleWriterExec: partitioning: None
```
while the sibling writer in the same file reports something real:
```
SortShuffleWriterExec: partitioning=Hash([c_custkey@0], 16)
```
## Expected behavior
`ShuffleWriterExec` reports its actual output partitioning, the same as
`SortShuffleWriterExec` does, so `EXPLAIN` output and the plan-stability
goldens say something true and the partition count is visible.
## Additional context
The goldens regenerate with `BALLISTA_GENERATE_GOLDEN=1 cargo test -p
ballista-scheduler --test tpch_plan_stability`, and the harness plans against a
synthetic stats table, so no TPC-H data is needed. The resulting diff is large
but mechanical.
Two related observations, not proposed as part of the fix:
- The two writers use different separators, `partitioning: ` versus
`partitioning=`, so they read inconsistently side by side in the same plan.
- `ShuffleWriterExec` also has a plain `impl std::fmt::Display` (line 384)
printing job id, stage id, work dir and the full child plan. Two display paths
with very different content, and the informative one is not the one `EXPLAIN`
uses.
--
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]
