[PR] Review follow-ups for the DataFusion 55 upgrade [datafusion-ballista]

["avantgardnerio (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] Review follow-ups for the DataFusion 55 upgrade [datafusion-ballista]

["andygrove (via GitHub)" <gi...@apache.org>]

andygrove merged PR #2295:
URL: https://github.com/apache/datafusion-ballista/pull/2295


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org