avantgardnerio opened a new pull request, #2295:
URL: https://github.com/apache/datafusion-ballista/pull/2295
Review follow-ups for #2241, targeting that branch so they land with it.
Opened as a draft: more commits are coming as I work through the rest of the
review, and CI on this branch is doing the eight-way clippy matrix for us in
the meantime.
## Verification done on #2241 before opening this
- `cargo build` and `ci/scripts/rust_clippy.sh` clean.
- Scheduler unit tests pass, including all 8 `parallel_window` plan-shape
tests.
- h2o Q8 (`RANGE BETWEEN`, no `PARTITION BY`) runs end to end on a
2-executor local cluster with the full parallel-window shape intact:
`OrderedRangeRepartitionExec` -> `RangeShuffleReaderExec` ->
`RangeFilterExec(halo=[3, 0])` -> `PartitionedBoundedWindowAggExec` ->
`RangeFilterExec(halo=[0, 0])`, cuts resolved from the runtime sketch.
- TPC-H SF100, 21/21 queries pass on the same cluster (q16 skipped as
usual). Timings not comparable: the lineitem-shuffling queries were spill-bound
at the default 256MB per-task sort-shuffle budget.
Results were not verified against the DataFusion oracle, so this is
pass/fail and plan shape only.
## Commits
### `fix(scheduler): refuse to restrict a Range-partitioned shuffle reader`
`select_output_partitions` gained a `Partitioning::Range` arm that rewrites
the partitioning to `UnknownPartitioning` while still slicing the partition
list. That hands back a `ShuffleReaderExec` which under-reports its own
partitioning and has silently dropped its split points.
The comment says we should never encounter it, but DataFusion 55 does
construct `Partitioning::Range` in live code: `physical_planner.rs` builds one
when lowering a logical range `Repartition`, and `ListingTable` uses it for
table output partitioning. So it is unmapped rather than unreachable, which
argues for failing loudly.
This returns an error naming the stage, with a TODO describing what support
would take: slicing the split points alongside the partition slice, the way the
`RangeFilterExec` arm slices `raw_bounds`.
`select_output_partitions` now returns `Result<Option<_>>`. Every other exit
keeps its existing behaviour - the `.ok()?` sites became explicit `let ... else
{ return Ok(None) }` - so only the `Range` arm changed.
## Two review points that are not code changes
**`cargo publish` and the git dependency.** No Ballista crate sets `publish
= false`, and `cargo publish` rejects git dependencies. From this merge until
the pin returns to a crates.io release, `main` cannot cut a release, which also
bears on an ASF release vote. Worth stating that window explicitly in #2241's
description. What is the intended plan if a release is needed before DataFusion
55 ships?
**`EnforceSorting` -> `EnsureRequirements` is not a rename.**
`EnforceSorting` no longer exists standalone in rc2, so the move is forced, but
`EnsureRequirements` also runs distribution enforcement, and the planner
applies it to stage inputs *after* stage splitting. The snapshot in
`planner.rs` picks this up: a stage that previously had no `RepartitionExec`
now gains `RepartitionExec: partitioning=RoundRobinBatch(2)`.
Production is partly shielded, since `session_manager` forces
`enable_round_robin_repartition=false`, and I confirmed no `RepartitionExec`
was injected during the h2o and TPC-H runs above. But the distribution half is
not gated: post-split, the rule can insert a `RepartitionExec::Hash` or
`CoalescePartitionsExec` *inside* a stage where the pre-split planner would
have created a shuffle boundary. That also feeds
`walk_child_partition_mapping`, which returns `KSpace` as soon as any
`RepartitionExec` appears in the writer's child chain, so a stage that
previously mapped `PassThrough(global ids)` would flip to local-equals-global
identity.
Is the added in-stage distribution enforcement intended here, or should the
rule be constrained to the sorting behaviour the old call had?
--
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]
