kosiew commented on code in PR #23785:
URL: https://github.com/apache/datafusion/pull/23785#discussion_r3664264762
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -1352,14 +1359,65 @@ mod tests {
}
}
+ /// End-to-end coverage for `Float16` group keys. Beyond routing through
the
+ /// column-wise `GroupValuesColumn` fast path, this exercises the float
+ /// canonicalization the builder relies on (shared with Float32 / Float64
via
+ /// `HashValue`): `-0.0` and `+0.0` collapse into one group stored as
`+0.0`,
+ /// and equal `NaN`s collapse into a single group.
+ #[test]
+ fn test_group_values_column_float16() {
+ use half::f16;
+
+ let schema =
Review Comment:
Nice test! One small suggestion: since the user-visible regression is the
all-or-nothing multi-column gate, it might be worth making this schema
`(Float16, Int32)` instead of a single `Float16` column.
That would let the test assert that `(-0.0, 3)` and `(+0.0, 3)` resolve to
the same group, while values like `(0.0, 4)` remain distinct because the
`Int32` key differs. The SLT already verifies the SQL behaviour, but this would
exercise the exact multi-column `GroupValuesColumn` path and make the gate and
factory integration a little more explicit.
--
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]
