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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
