berkaysynnada commented on PR #13897:
URL: https://github.com/apache/datafusion/pull/13897#issuecomment-2561839488
> I recommend some tests if possible to avoid breakages during future
refactorings
Unless some downstream rules explicitly check the `maintains_input_order`
flag of the ExecutionPlan (which we do in our fork), this implementation does
not benefit to the current code.
> If we add a unit test with an unnecessary SortExec on top of an
order-maintaining AggregateExec, would the EnforceSorting rule remove it with
this change? I guess it wouldn't remove it before?
It wouldn't remove it, since the current upstream rules (like
EnforceSorting) are using EquivalenceProperties::ordering_satisfy() API, and
that one is consulting the AggregateExec output ordering (or related property
cache of any operator), not the `maintains_input_order()`. The critical point
here is that output ordering is built from directly group_by columns again, not
using maintains_input_order() at all.
```rust
let projection_mapping =
ProjectionMapping::try_new(&group_by.expr, &input.schema())?;
...
let cache = Self::compute_properties(
&input,
Arc::clone(&schema),
&projection_mapping,
&mode,
&input_order_mode,
);
```
```rust
pub fn compute_properties(
input: &Arc<dyn ExecutionPlan>,
schema: SchemaRef,
projection_mapping: &ProjectionMapping,
mode: &AggregateMode,
input_order_mode: &InputOrderMode,
) -> PlanProperties {
// Construct equivalence properties:
let eq_properties = input
.equivalence_properties()
.project(projection_mapping, schema);
```
--
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]
