Skip to content

Conversation

@jonahgao
Copy link
Member

Which issue does this PR close?

N/A

Rationale for this change

In the call to find_aggregate_exprs, we copied the select list expressions and the having expression.

What changes are included in this PR?

Are these changes tested?

Yes

Are there any user-facing changes?

No

The public API find_aggregate_exprs becomes generalized and will not break existing calls.

@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions labels Oct 12, 2024
/// They are returned in order of occurrence (depth
/// first), with duplicates omitted.
pub fn find_aggregate_exprs(exprs: &[Expr]) -> Vec<Expr> {
pub fn find_aggregate_exprs<'a>(exprs: impl IntoIterator<Item = &'a Expr>) -> Vec<Expr> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jonahgao and @findepi

@alamb alamb merged commit 849bbe7 into apache:main Oct 14, 2024
hailelagi pushed a commit to hailelagi/datafusion that referenced this pull request Oct 14, 2024
@jonahgao jonahgao deleted the remove_clone branch October 16, 2024 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-expr Logical plan and expressions sql SQL Planner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants