alamb commented on code in PR #22751:
URL: https://github.com/apache/datafusion/pull/22751#discussion_r3374373260
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -1219,7 +1201,13 @@ pub fn supported_schema(schema: &Schema) -> bool {
/// Returns true if the specified data type is supported by
[`GroupValuesColumn`]
///
/// In order to be supported, there must be a specialized implementation of
-/// [`GroupColumn`] for the data type, instantiated in
[`GroupValuesColumn::intern`]
+/// [`GroupColumn`] for the data type, instantiated in
+/// [`make_group_column`]. This function is the allow-list that gates the
+/// `GroupValuesRows` fallback in
[`crate::aggregates::group_values::new_group_values`];
+/// it must accept exactly the set of types that [`make_group_column`]
+/// constructs a builder for. The
+/// `supported_type_and_make_group_column_stay_in_sync` test below pins
+/// this biconditional.
fn supported_type(data_type: &DataType) -> bool {
Review Comment:
Maybe we coul also make this function named something that reflects it is
gating `make_group_column` -- somthing like `group_column_supported_type` and
move it closer to `make_group_column` 🤔
--
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]
