[GitHub] [arrow-datafusion] Jefffrey commented on a diff in pull request #5485: AggregateStatistics min/max/count take_optimizable replace col_expr name with casted_expr name

["Jefffrey (via GitHub)" <gi...@apache.org>]

Jefffrey commented on code in PR #5485:
URL: https://github.com/apache/arrow-datafusion/pull/5485#discussion_r1128529608


##########
datafusion/core/tests/sql/aggregates.rs:
##########
@@ -99,11 +99,11 @@ async fn aggregate_timestamps_count() -> Result<()> {
     .await;
 
     let expected = vec![
-        "+--------------+---------------+---------------+-------------+",
-        "| COUNT(nanos) | COUNT(micros) | COUNT(millis) | COUNT(secs) |",
-        "+--------------+---------------+---------------+-------------+",
-        "| 3            | 3             | 3             | 3           |",
-        "+--------------+---------------+---------------+-------------+",
+        "+----------------+-----------------+-----------------+---------------+",
+        "| COUNT(t.nanos) | COUNT(t.micros) | COUNT(t.millis) | COUNT(t.secs) |",
+        "+----------------+-----------------+-----------------+---------------+",
+        "| 3              | 3               | 3               | 3             |",
+        "+----------------+-----------------+-----------------+---------------+",

Review Comment:
   i feel this might be an unwanted change, since it doesn't seem intuitive to do a `select count(nanos)` and have the result be `COUNT(t.nanos)` in my opinion 🤔
   
   especially as i believe this might be only for counts which utilize aggregate statistics rule? so regular counts would still output `COUNT(nanos)` i believe?



-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org