jayzhan211 commented on code in PR #24698:
URL: https://github.com/apache/datafusion/pull/24698#discussion_r4226027432
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1781,17 +1786,19 @@ impl AggregateExec {
group_expr_mapping: &ProjectionMapping,
is_true_no_grouping: bool,
mode: &AggregateMode,
- input_order_mode: &InputOrderMode,
+ group_clustering_mode: &GroupClusteringMode,
aggr_exprs: &[Arc<AggregateFunctionExpr>],
) -> Result<PlanProperties> {
// Construct equivalence properties:
let mut eq_properties = input
.equivalence_properties()
.project(group_expr_mapping, schema);
Review Comment:
We might need this
`eq_properties.clear_groupings();`
##########
datafusion/expr-common/src/sort_properties.rs:
##########
@@ -37,16 +37,24 @@ use arrow::datatypes::DataType;
pub enum SortProperties {
/// Use the ordinary [`SortOptions`] struct to represent ordered data:
Ordered(SortOptions),
- // This alternative represents unordered data:
+ /// Within each partition, all rows with the same value for this expression
+ /// form one contiguous run. The runs may occur in any order.
+ Grouped,
Review Comment:
`SortProperties::Grouped` is only read by `projected_groupings`. #24497
never uses it, and `grouping_satisfy([CAST(a AS BIGINT)])` returns false even
when `get_expr_properties` reports `Grouped`. With the variant disabled on
#24497's head (de77a8a2e8), all 309 aggregate tests pass and the #24438 shape
(`[key, date_bin(time)]`, with or without a `ProjectionExec`) still gets
`GroupClusteringMode::Full`. The only case lost is a single grouped key
projected through `CAST(key AS BIGINT) AS k2`.
The variant is also the only semver break in this PR (`SortProperties` and
`FFI_SortProperties`, with no upgrade-guide entry yet), and downstream `!=
Unordered` checks would treat it as ordered, as `join_selection.rs` did here.
Suggest removing it from this PR rather than shipping it in 56.0.0, since
taking it out after release would be a second breaking change. If the
cast-through-projection case is needed later, it can be proposed with that use
case, together with `grouping_satisfy` support.
--
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]
