-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Support no distinct aggregate sum/min/max in single_distinct_to_group_by
rule
#8124
Conversation
for e in args { | ||
fields_set.insert(e.canonical_name()); | ||
match filter { | ||
Some(_) => return Ok(false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this pr, we also don't support filter in single_distinct_to_group_by
rule. But we forget check it.
The behaviors of I test on this branch: DataFusion CLI v33.0.0
❯ create table t(a int, b int);
0 rows in set. Query took 0.016 seconds.
❯ select count(distinct a), count(b) from t;
+---------------------+------------+
| COUNT(DISTINCT t.a) | COUNT(t.b) |
+---------------------+------------+
| 0 | |
+---------------------+------------+
1 row in set. Query took 0.029 seconds.
Maybe we need a special |
@jonahgao thanks for your provider this case. I didn't read the source code of calcite carefully.
I have no preference between the two methods, what do you think @alamb ? |
I hope to review this PR later today |
single_distinct_to_group_by
rulesingle_distinct_to_group_by
rule
I am sorry -- I did not have a chance to get to this one. Note that this PR seems to have non trivial conflicts now (perhaps due to #8176 |
Due to #8176, I close this pr, and then reorganize the code and submit a new pr. |
Which issue does this PR close?
Closes #8123
Rationale for this change
In this pr
in main 43cc870
What changes are included in this PR?
add no-distinct sum/min/max aggregate support in
single_distinct_to_group_by
ruleAre these changes tested?
yes, add soem tests
Are there any user-facing changes?