adriangb commented on code in PR #21739:
URL: https://github.com/apache/datafusion/pull/21739#discussion_r3209583227
##########
datafusion/physical-expr/src/aggregate.rs:
##########
@@ -370,6 +388,210 @@ impl AggregateExprBuilder {
}
}
+#[derive(Debug, Clone)]
+struct LoweredAggregateHumanDisplay {
+ expression: String,
+ alias: Option<String>,
+}
+
+/// Result of lowering a logical aggregate expression to physical aggregate
+/// pieces.
+#[derive(Debug, Clone)]
+pub struct LoweredAggregate {
+ pub aggregate: Arc<AggregateFunctionExpr>,
+ pub filter: Option<Arc<dyn PhysicalExpr>>,
+ pub order_bys: Vec<PhysicalSortExpr>,
+}
+
+/// Builder for lowering a logical aggregate [`Expr`] to physical aggregate
+/// pieces.
+pub struct LoweredAggregateBuilder<'a> {
+ expr: &'a Expr,
+ name: Option<String>,
+ human_display: Option<LoweredAggregateHumanDisplay>,
+ output_metadata: Option<FieldMetadata>,
+ preserve_alias_metadata: bool,
+ logical_input_schema: &'a DFSchema,
+ physical_input_schema: &'a Schema,
+ execution_props: &'a ExecutionProps,
+}
+
+impl<'a> LoweredAggregateBuilder<'a> {
+ pub fn new(
+ expr: &'a Expr,
+ logical_input_schema: &'a DFSchema,
+ physical_input_schema: &'a Schema,
+ execution_props: &'a ExecutionProps,
+ ) -> Self {
+ Self {
+ expr,
+ name: None,
+ human_display: None,
+ output_metadata: None,
+ preserve_alias_metadata: true,
+ logical_input_schema,
+ physical_input_schema,
+ execution_props,
+ }
+ }
+
+ pub fn with_name(mut self, name: impl Into<String>) -> Self {
+ self.name = Some(name.into());
+ self
+ }
+
+ #[doc(hidden)]
Review Comment:
Why hidden?
##########
datafusion/physical-expr/src/aggregate.rs:
##########
@@ -370,6 +388,210 @@ impl AggregateExprBuilder {
}
}
+#[derive(Debug, Clone)]
+struct LoweredAggregateHumanDisplay {
+ expression: String,
+ alias: Option<String>,
+}
+
+/// Result of lowering a logical aggregate expression to physical aggregate
+/// pieces.
+#[derive(Debug, Clone)]
+pub struct LoweredAggregate {
+ pub aggregate: Arc<AggregateFunctionExpr>,
+ pub filter: Option<Arc<dyn PhysicalExpr>>,
+ pub order_bys: Vec<PhysicalSortExpr>,
+}
+
+/// Builder for lowering a logical aggregate [`Expr`] to physical aggregate
+/// pieces.
+pub struct LoweredAggregateBuilder<'a> {
+ expr: &'a Expr,
+ name: Option<String>,
+ human_display: Option<LoweredAggregateHumanDisplay>,
+ output_metadata: Option<FieldMetadata>,
+ preserve_alias_metadata: bool,
+ logical_input_schema: &'a DFSchema,
+ physical_input_schema: &'a Schema,
+ execution_props: &'a ExecutionProps,
+}
+
+impl<'a> LoweredAggregateBuilder<'a> {
+ pub fn new(
+ expr: &'a Expr,
+ logical_input_schema: &'a DFSchema,
+ physical_input_schema: &'a Schema,
+ execution_props: &'a ExecutionProps,
+ ) -> Self {
+ Self {
+ expr,
+ name: None,
+ human_display: None,
+ output_metadata: None,
+ preserve_alias_metadata: true,
+ logical_input_schema,
+ physical_input_schema,
+ execution_props,
+ }
+ }
+
+ pub fn with_name(mut self, name: impl Into<String>) -> Self {
Review Comment:
We should add docstrings to all of these. Ideally I can read the docstrings
on these functions / structs with general DataFusion knowledge but without
fully understanding how planning works and get some idea of what these do.
##########
datafusion/physical-expr/src/aggregate.rs:
##########
@@ -871,3 +1100,42 @@ fn replace_fn_name_clause(aggr_name: &mut String,
fn_name_old: &str, fn_name_new
*aggr_name = format!("{fn_name_new}{rest}");
}
}
+
+#[cfg(test)]
+mod tests {
Review Comment:
I feel like we can probably port other unit tests to test the new builder
directly? It seems like a good unit of code to unit test. Can be a followup /
when we remove the old methods, but especially any tests testing the now
deprecated methods are good candidates.
--
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]
