Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Min/Max references from AggregateExec::get_minmax_descr #11152

Closed
Tracked by #11151
alamb opened this issue Jun 28, 2024 · 1 comment · Fixed by #11257
Closed
Tracked by #11151

Remove Min/Max references from AggregateExec::get_minmax_descr #11152

alamb opened this issue Jun 28, 2024 · 1 comment · Fixed by #11257
Assignees
Labels
enhancement New feature or request

Comments

@alamb
Copy link
Contributor

alamb commented Jun 28, 2024

Is your feature request related to a problem or challenge?

Part of #11151 (where we are removing special case uses of Min/Max)

Describe the solution you'd like

Remove the code here:

pub fn get_minmax_desc(&self) -> Option<(Field, bool)> {
let agg_expr = self.aggr_expr.iter().exactly_one().ok()?;
if let Some(max) = agg_expr.as_any().downcast_ref::<Max>() {
Some((max.field().ok()?, true))
} else if let Some(min) = agg_expr.as_any().downcast_ref::<Min>() {
Some((min.field().ok()?, false))
} else {
None
}
}

Specifically, the idea is to remove the pattern

       expr.as_any().downcast_ref::<Max>()

and

       expr.as_any().downcast_ref::<Min>()

Describe alternatives you've considered

Perhaps we can add function like this to AggregteExpr and implement it for Min and Max:

impl AggregateExpr {
    /// If this function is max, return (output_field, true)
    /// if the function is min, return (output_field, false)
    /// otherwise return None (the default)
    ///
    /// output_field is the name of the column produced by this aggregate
    ///
    /// Note: this is used to use special aggregate implementations in certain conditions
    pub fn get_min_max(&self) -> Option<(Field, bool)> { None }
...
}

Additional context

No response

@alamb alamb added the enhancement New feature or request label Jun 28, 2024
@alamb alamb changed the title Remove Min/Max references from AggregateExec::get_minmax_field Remove Min/Max references from AggregateExec::get_minmax_descr Jun 28, 2024
@Rachelint
Copy link
Contributor

take

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants