zhuqi-lucas commented on code in PR #22751:
URL: https://github.com/apache/datafusion/pull/22751#discussion_r3380900778
##########
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:
Good idea — done in 1b3d8326e:
- Renamed `supported_type` -> `group_column_supported_type`
- Moved it to immediately precede `make_group_column` so the gate sits next
to the dispatcher
- Renamed the consistency test to
`group_column_supported_type_matches_make_group_column` and updated all
assertion messages
`supported_schema` (the public entry point used by `new_group_values`) keeps
its name.
--
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]
