alamb commented on code in PR #14824:
URL: https://github.com/apache/datafusion/pull/14824#discussion_r1967849825
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -6296,6 +6299,54 @@ physical_plan
01)AggregateExec: mode=Single, gby=[], aggr=[count(NULL)]
02)--DataSourceExec: partitions=1, partition_sizes=[1]
+statement count 0
+drop table t;
+
+# test duplicated shema name issue
+
+statement count 0
+create table t (a int) as values (1), (2);
+
+query I
+select count() from t;
+----
+2
+
+query I
+select count(1) * count(2) from t;
+----
+4
+
+query I
+select count(1) * count(*) from t;
+----
+4
+
+query I
+select count(*) * count(*) from t;
+----
+4
+
+query I
Review Comment:
Likewise here it would be nice to have `count(1)` and `count(2)`
individually tested
##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -1460,13 +1460,13 @@ fn
select_simple_aggregate_with_groupby_and_column_is_in_aggregate_and_groupby()
#[test]
fn select_simple_aggregate_with_groupby_can_use_positions() {
quick_test(
- "SELECT state, age AS b, count(1) FROM person GROUP BY 1, 2",
+ "SELECT state, age AS b, count() FROM person GROUP BY 1, 2",
Review Comment:
Why is this test changed?
##########
datafusion/functions-aggregate/src/count.rs:
##########
@@ -550,8 +571,6 @@ impl AggregateUDFImpl for Count {
fn is_count_wildcard(args: &[Expr]) -> bool {
Review Comment:
this function now feels a bit redundant as it is just checking for `.empty()`
##########
datafusion/sqllogictest/test_files/count_star_rule.slt:
##########
@@ -80,12 +80,12 @@ query TT
EXPLAIN SELECT a, COUNT() OVER (PARTITION BY a) AS count_a FROM t1;
----
logical_plan
-01)Projection: t1.a, count(*) PARTITION BY [t1.a] ROWS BETWEEN UNBOUNDED
PRECEDING AND UNBOUNDED FOLLOWING AS count_a
-02)--WindowAggr: windowExpr=[[count(*) PARTITION BY [t1.a] ROWS BETWEEN
UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING]]
+01)Projection: t1.a, count(Int64(1)) PARTITION BY [t1.a] ROWS BETWEEN
UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING AS count_a
Review Comment:
Given you implemented support for `count()` I don't understand why this is
this changed to `count(1)` (why isn't it count()?`
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -6296,6 +6299,54 @@ physical_plan
01)AggregateExec: mode=Single, gby=[], aggr=[count(NULL)]
02)--DataSourceExec: partitions=1, partition_sizes=[1]
+statement count 0
+drop table t;
+
+# test duplicated shema name issue
+
+statement count 0
+create table t (a int) as values (1), (2);
+
+query I
+select count() from t;
+----
+2
+
+query I
+select count(1) * count(2) from t;
Review Comment:
Could you also please add a test that shows just the values of `count(2)`
For example
```sql
select count(1), count(2), count(1) * count(2) from t;
```
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
