wirybeaver commented on PR #1722: URL: https://github.com/apache/datafusion-ballista/pull/1722#issuecomment-4505764269
@milenkovicm Thanks for pushing back — your questions exposed that my reasoning was wrong. Let me walk through what I now see. **The flawed premise.** I assumed `ExchangeExec::equivalence_properties()` would reflect post-shuffle ordering once the exchange resolves. It doesn't. From `ballista/scheduler/src/state/aqe/execution_plan.rs:118`: ```rust let eq_properties = input.properties().eq_properties.clone(); ``` The eq_properties are captured at construction time from the **pre-shuffle input** and never updated when `resolve_shuffle_partitions()` is called. **Why the rule is a no-op (or worse).** 1. **Linear → Sorted can't usefully fire.** If the agg's input advertises sort properties, those were visible at the agg's construction time too — `try_new_with_schema` derived the right mode then. The rule's `with_new_children` re-derivation produces the same mode. 2. **When it would fire, it would be incorrect.** A hash repartition wrapping a `SortExec` carries the sort eq_properties on the exchange, even though the actual post-shuffle data is no longer globally sorted (hash shuffle destroys global ordering). Switching the agg to `Sorted` based on those properties would cause streaming aggregation to produce wrong results. 3. **In valid replan scenarios it's redundant.** If a sibling rule changes the agg's subtree during `transform_up`, the framework already calls `with_new_children` on the parent, which re-derives the mode — no separate rule needed. **What the roadmap line actually requires.** For AQE to re-derive useful information after a stage resolves, the resolved exchange (or whatever node represents the post-shuffle reader) needs to advertise *post-shuffle* properties — driven by shuffle mode: - Hash shuffle: destroys global ordering, only preserves partitioning on the hash key - Sort-based shuffle: preserves per-partition ordering on the sort key - Single-partition coalesce: preserves input ordering Once those properties reflect reality, no dedicated re-derivation rule is needed — `with_new_children` during normal optimizer passes naturally picks up the change. **Proposal.** Close this PR. I'd like to open a separate one (or RFC discussion) to update `ExchangeExec.equivalence_properties()` to translate properties on resolve based on shuffle mode. Happy to take direction from you on whether that's the right next step or if I'm still misreading the AQE design. Apologies for the misfire — should have validated the premise with a runnable example before opening. -- 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]
