sdf-jkl commented on code in PR #26939:
URL: https://github.com/apache/datafusion/pull/26939#discussion_r4444741539
##########
datafusion/physical-optimizer/src/aggregate_statistics.rs:
##########
@@ -67,8 +69,14 @@ impl PhysicalOptimizerRule for AggregateStatistics {
if let Some((optimizable_statistic, name)) =
take_optimizable_value_from_statistics(&statistics_args,
expr)
{
+ // Keep the metadata of the aggregate field, such as an
extension type
+ let metadata = Some(FieldMetadata::from(field.as_ref()))
+ .filter(|metadata| !metadata.is_empty());
projections.push(ProjectionExpr {
- expr: expressions::lit(optimizable_statistic),
+ expr: Arc::new(Literal::new_with_metadata(
+ optimizable_statistic,
+ metadata,
Review Comment:
Non-blocking: `Literal::new_with_metadata` already calls
`FieldMetadata::add_to_field`, which treats empty metadata as a no-op. We can
pass the metadata directly and remove the duplicate empty check and temporary
binding.
```suggestion
projections.push(ProjectionExpr {
expr: Arc::new(Literal::new_with_metadata(
optimizable_statistic,
Some(FieldMetadata::from(field.as_ref())),
```
##########
datafusion/functions-aggregate/src/min_max.rs:
##########
@@ -207,6 +208,18 @@ impl FromColumnStatistics for Max {
}
}
+/// Returns the field of the result of `min` or `max`. The result has the type
of the argument,
+/// so it keeps the metadata of the argument field, such as an extension type.
+fn min_max_return_field(
+ func: &impl AggregateUDFImpl,
+ arg_fields: &[FieldRef],
+) -> Result<FieldRef> {
+ let field = udaf_default_return_field(func, arg_fields)?;
+ Ok(Arc::new(
+
Arc::unwrap_or_clone(field).with_metadata(arg_fields[0].metadata().clone()),
+ ))
Review Comment:
Non-blocking: could we reuse `FieldMetadata::add_to_field_ref` here? It
already applies metadata with `Arc::make_mut`, avoiding the unwrap and second
Arc allocation. Empty metadata is equivalent here because the default field has
none. The companion import suggestion is above.
```suggestion
Ok(FieldMetadata::from(arg_fields[0].as_ref()).add_to_field_ref(field))
```
##########
datafusion/functions-aggregate/src/min_max.rs:
##########
@@ -9,22 +9,23 @@ mod min_max_bytes;
mod min_max_struct;
use arrow::array::{ArrayRef, ArrowPrimitiveType, downcast_primitive};
-use arrow::datatypes::{DataType, Float16Type, Float32Type, Float64Type};
+use arrow::datatypes::{DataType, FieldRef, Float16Type, Float32Type,
Float64Type};
use datafusion_common::float_semantics::{float_gt, float_lt};
use datafusion_common::stats::Precision;
use datafusion_common::{ColumnStatistics, Result, exec_err, internal_err};
use
datafusion_functions_aggregate_common::aggregate::groups_accumulator::prim_op::PrimitiveGroupsAccumulator;
use datafusion_physical_expr::expressions;
use std::cmp::Ordering;
use std::fmt::Debug;
+use std::sync::Arc;
Review Comment:
Companion import change for the `add_to_field_ref` suggestion below: add
`FieldMetadata` and remove the now-unused `Arc` import. Apply these two
suggestions together.
```suggestion
use datafusion_common::float_semantics::{float_gt, float_lt};
use datafusion_common::metadata::FieldMetadata;
use datafusion_common::stats::Precision;
use datafusion_common::{ColumnStatistics, Result, exec_err, internal_err};
use
datafusion_functions_aggregate_common::aggregate::groups_accumulator::prim_op::PrimitiveGroupsAccumulator;
use datafusion_physical_expr::expressions;
use std::cmp::Ordering;
use std::fmt::Debug;
```
--
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]
