andygrove opened a new pull request, #2307:
URL: https://github.com/apache/datafusion-ballista/pull/2307
# Which issue does this PR close?
Closes #2306.
# Rationale for this change
`ShuffleWriterExec`'s `DisplayAs` wrote a string literal rather than a value:
```rust
// "None" is retained for plan-shape stability: this writer never
// repartitions, so the value can only ever be None.
write!(f, "ShuffleWriterExec: partitioning: None")
```
It used to print a value. #2106 removed the `shuffle_output_partitioning`
field along with the hash-partitioning writer, and the token was frozen as a
literal so the 22 approved TPC-H goldens would not churn in the same diff. That
was a fine expedient, but it left the output carrying no information and kept
*because* it carries none: a constant cannot drift, so it cannot break a golden
file.
It was also misleading. The `None` refers to the absence of a repartitioning
scheme, an internal distinction a plan reader cannot infer. The writer
preserves its input's partitioning, and `try_new` already sets its
`PlanProperties` to exactly that, so the real value was available the whole
time.
There was a downstream cost too. #2305 labels DOT graph nodes from each
operator's `DisplayAs`, which works everywhere except here: because the text
was a constant, that PR had to keep a hand-written special case for
`ShuffleWriterExec` to recover the partition count.
# What changes are included in this PR?
- Render `self.properties().output_partitioning()` in both
`Default`/`Verbose` and `TreeRender`, matching how `SortShuffleWriterExec`
already reports its own.
- Add `display_as_reports_real_partitioning`, which renders two writers over
inputs with different partitioning and asserts each reflects its own. A
constant cannot pass it.
- Regenerate the 22 approved goldens with `BALLISTA_GENERATE_GOLDEN=1`.
The golden diff is 45 lines and entirely mechanical. Each new value is
checkable against the child plan on the line below it, for example in `q1.txt`:
```
=== Stage 2 ===
ShuffleWriterExec: partitioning: Hash([l_returnflag@0, l_linestatus@1], 16)
...
UnresolvedShuffleExec: stage=1, partitioning: Hash([l_returnflag@0,
l_linestatus@1], 16)
=== Stage 3 ===
ShuffleWriterExec: partitioning: UnknownPartitioning(1)
SortPreservingMergeExec: [l_returnflag@0 ASC NULLS LAST, l_linestatus@1
ASC NULLS LAST]
```
Stage 2 inherits the hash partitioning it writes; stage 3 collapsed to one
partition through the merge. Both were previously reported as `None`.
# Are there any user-facing changes?
`EXPLAIN` output and the scheduler's rendered stage plans now show the
writer's actual output partitioning instead of a constant `None`. No public API
changes.
Two related warts noted in the issue are deliberately left alone here, since
neither is needed for the fix: the two writers use different separators
(`partitioning: ` versus `partitioning=`), and `ShuffleWriterExec` also carries
a plain `impl Display` whose content differs from its `DisplayAs`.
Once this lands, the `ShuffleWriterExec` special case in #2305 can be
dropped and that operator labeled generically like every other one.
--
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]
