kosiew commented on code in PR #23783:
URL: https://github.com/apache/datafusion/pull/23783#discussion_r3664157139
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -1352,6 +1377,56 @@ mod tests {
}
}
+ /// End-to-end coverage for `Duration` group keys: a `Duration` column
stays
+ /// on the `GroupValuesColumn` fast path, deduplicates equal durations
+ /// (including nulls), and round-trips with its `Duration` output type
+ /// preserved (not the bare `i64` native). The four units share one
builder,
+ /// so a single unit exercises the dispatcher; the fuzz above covers
routing
+ /// for every unit.
+ #[test]
+ fn test_group_values_column_duration() {
+ use arrow::datatypes::TimeUnit;
+
+ let schema = Arc::new(Schema::new(vec![Field::new(
Review Comment:
Nice addition to cover `Duration` group keys. One small thought: this
regression is specifically about the multi-column all-or-nothing gate, but this
unit test exercises a single-column `Duration` `GroupValuesColumn`. The SQL
logic test already covers the multi-column behaviour end to end, but it might
be worth making this unit test use `(Duration(Microsecond), Int64)` input, or
adding a small second unit test that does. That would exercise the
`GroupValuesColumn` helper at the same boundary as the fix and make the
regression a little more direct.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
