Skip to content

Count agg support multiple expressions - #5908

Merged
yahoNanJing merged 2 commits into
apache:mainfrom
allenma:count_mul_expr
Apr 10, 2023
Merged

Count agg support multiple expressions#5908
yahoNanJing merged 2 commits into
apache:mainfrom
allenma:count_mul_expr

Conversation

@allenma

@allenmaallenma commented Apr 7, 2023

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes#5619

Rationale for this change

Most of other sql engines like pg, mysql, spark support count for multiple expressions like: select count(col1, col2) from table1;

What changes are included in this PR?

Support count for multiple expressions, when any expression return null for each row, the row will not be counted.

Are these changes tested?

Yes, add new ut and e2e test

Are there any user-facing changes?

No

No

@github-actionsgithub-actionsBot added core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates labels Apr 7, 2023
@allenma
allenmaforce-pushed the count_mul_expr branch 2 times, most recently from 3508b98 to 1262c52CompareApril 7, 2023 09:50
ozankabak
ozankabak approved these changes Apr 7, 2023
View reviewed changes

@ozankabakozankabak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some minor comments inline, looks good to me in general. Thank you!

Comment threaddatafusion/expr/src/type_coercion/aggregates.rs Outdated
Comment threaddatafusion/expr/src/type_coercion/functions.rs Outdated
Comment threaddatafusion/physical-expr/src/aggregate/count.rs
Comment threaddatafusion/expr/src/signature.rs Outdated
Comment threaddatafusion/expr/src/signature.rs Outdated
Comment threaddatafusion/core/tests/sql/errors.rs Outdated
@allenma

allenma commented Apr 8, 2023

Copy link
Copy Markdown
ContributorAuthor

Thank you @ozankabak for your careful review, have addressed all your comments, please confirm

@ozankabak

ozankabak commented Apr 8, 2023

Copy link
Copy Markdown
Contributor

LGTM

yahoNanJing
yahoNanJing reviewed Apr 10, 2023
View reviewed changes

@yahoNanJingyahoNanJing left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@mingmwang
mingmwang self-requested a review April 10, 2023 04:03
@mingmwang

mingmwang commented Apr 10, 2023

Copy link
Copy Markdown
Contributor

LGTM

@yahoNanJing
yahoNanJing merged commit 524a3c5 into apache:mainApr 10, 2023
@mingmwang

mingmwang commented Apr 10, 2023

Copy link
Copy Markdown
Contributor

@allenma
Would you please add one more UT to cover the count multiple distinct cases ? You can do this in the following PR.

select count(distinct col1, col2) from demo;

@allenma

allenma commented Apr 10, 2023

Copy link
Copy Markdown
ContributorAuthor

#5619

@mingmwang Currently count distinct also don't support multiple expressions, will create another pr to address this.

@jonahgaojonahgao mentioned this pull request Jul 7, 2024
Closed
@rtylerrtyler mentioned this pull request Aug 15, 2024
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yahoNanJingyahoNanJingyahoNanJing left review comments
@ozankabakozankabakozankabak approved these changes
@mingmwangmingmwangAwaiting requested review from mingmwang

Assignees

No one assigned

Labels

coreCore DataFusion cratelogical-exprLogical plan and expressionsphysical-exprChanges to the physical-expr crates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Count() and Count(Distinct )should accept multiple exprs

4 participants

@allenma@ozankabak@mingmwang@yahoNanJing