Rich-T-kid commented on code in PR #23405:
URL: https://github.com/apache/datafusion/pull/23405#discussion_r3559519068
##########
datafusion/physical-plan/src/aggregates/group_values/mod.rs:
##########
@@ -23,11 +23,70 @@ use arrow::array::types::{
TimestampMillisecondType, TimestampNanosecondType, TimestampSecondType,
};
use arrow::array::{ArrayRef, downcast_primitive};
-use arrow::datatypes::{DataType, SchemaRef, TimeUnit};
+use arrow::datatypes::{DataType, Field, FieldRef, Schema, SchemaRef, TimeUnit};
use datafusion_common::Result;
+use std::sync::Arc;
use datafusion_expr::EmitTo;
+pub(crate) fn group_value_output_schema(schema: &Schema) -> SchemaRef {
+ Arc::new(Schema::new(
+ schema
+ .fields()
+ .iter()
+ .map(|field| group_value_output_field(field.as_ref()))
+ .collect::<Vec<_>>(),
+ ))
+}
+
+pub(crate) fn group_value_output_field(field: &Field) -> FieldRef {
+ Arc::new(
+ Field::new(
+ field.name(),
+ group_value_output_data_type(field.data_type()),
+ field.is_nullable(),
+ )
+ .with_metadata(field.metadata().clone()),
+ )
+}
+
+fn group_value_output_data_type(data_type: &DataType) -> DataType {
+ match data_type {
+ DataType::Dictionary(_, value_type) => DataType::Dictionary(
+ Box::new(DataType::UInt64),
+ Box::new(group_value_output_data_type(value_type)),
+ ),
Review Comment:
🤔 I don't think making every dictionary key type `u64` is the best option
here. changing a key from `i8` to `u64` just because a group by had 128 groups
is overkill in my opinion.
##########
datafusion/physical-plan/src/aggregates/group_values/row.rs:
##########
@@ -102,8 +103,9 @@ impl GroupValuesRows {
let starting_data_capacity = 64 * starting_rows_capacity;
let rows_buffer =
row_converter.empty_rows(starting_rows_capacity,
starting_data_capacity);
+ let output_schema = group_value_output_schema(schema.as_ref());
Ok(Self {
- schema,
+ schema: output_schema,
Review Comment:
The root of the issue is here, we don't know how many groups we will end up
with until we've called `emit()`. to resolve #23127 I think we need to allow
for schemas to be somewhat dynamic. In the sense that a dictionary's key type
can group but no shrink.
--
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]
