You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "jayzhan211 (via GitHub)" <gi...@apache.org> on 2024/07/20 09:05:05 UTC
[PR] Move `OrderSensitiveArrayAgg` to UDAF [datafusion]
jayzhan211 opened a new pull request, #11564:
URL: https://github.com/apache/datafusion/pull/11564
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate `OrderSensitiveArrayAgg` to be a user defined aggregate [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #11564:
URL: https://github.com/apache/datafusion/pull/11564#discussion_r1687256081
##########
datafusion/physical-expr-common/src/aggregate/mod.rs:
##########
@@ -81,6 +86,61 @@ pub fn create_aggregate_expr(
.map(|e| e.expr.data_type(schema))
.collect::<Result<Vec<_>>>()?;
+ let ordering_fields = ordering_fields(ordering_req, &ordering_types);
+ let name = name.into();
+
+ Ok(Arc::new(AggregateFunctionExpr {
+ fun: fun.clone(),
+ args: input_phy_exprs.to_vec(),
+ logical_args: input_exprs.to_vec(),
+ data_type: fun.return_type(&input_exprs_types)?,
+ name,
+ schema: schema.clone(),
+ dfschema: DFSchema::empty(),
+ sort_exprs: sort_exprs.to_vec(),
+ ordering_req: ordering_req.to_vec(),
+ ignore_nulls,
+ ordering_fields,
+ is_distinct,
+ input_type: input_exprs_types[0].clone(),
+ is_reversed,
+ }))
+}
+
+#[allow(clippy::too_many_arguments)]
+// This is not for external usage, consider creating with `create_aggregate_expr` instead.
+pub fn create_aggregate_expr_with_dfschema(
Review Comment:
Great! Tracked in #11543.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Move `OrderSensitiveArrayAgg` to UDAF [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11564:
URL: https://github.com/apache/datafusion/pull/11564#discussion_r1687016111
##########
datafusion/expr/src/function.rs:
##########
@@ -57,6 +57,9 @@ pub struct AccumulatorArgs<'a> {
/// The schema of the input arguments
pub schema: &'a Schema,
+ /// The schema of the input arguments
Review Comment:
I view this addition as basically making the UDAF framework as full featured as the built in one is
##########
datafusion/physical-expr-common/src/aggregate/mod.rs:
##########
@@ -51,6 +51,10 @@ use datafusion_expr::utils::AggregateOrderSensitivity;
///
/// `input_exprs` and `sort_exprs` are used for customizing Accumulator as the arguments in `AccumulatorArgs`,
/// if you don't need them it is fine to pass empty slice `&[]`.
+///
+/// `is_reversed` is used to indicate whether the aggregation is running in reverse order,
+/// it could be used to hint Accumulator to accumulate in the reversed order,
+/// you can just set to false if you are not reversing expression
#[allow(clippy::too_many_arguments)]
pub fn create_aggregate_expr(
Review Comment:
Given the number of arguments it is getting, maybe it would be good to define a builder like the following:
```rust
let builder = AggregateExprBuilder::new(fun),
.with_input_phyiscal_exprs(input_phy_exprs)
...
.with_name(name)
.build()?
```
🤔
##########
datafusion/physical-expr-common/src/utils.rs:
##########
@@ -134,6 +135,33 @@ pub fn limited_convert_logical_expr_to_physical_expr(
}
}
+pub fn limited_convert_logical_expr_to_physical_expr_with_dfschema(
+ expr: &Expr,
+ dfschema: &DFSchema,
+) -> Result<Arc<dyn PhysicalExpr>> {
+ match expr {
+ Expr::Alias(Alias { expr, .. }) => Ok(
+ limited_convert_logical_expr_to_physical_expr_with_dfschema(expr, dfschema)?,
+ ),
+ Expr::Column(col) => {
+ let idx = dfschema.index_of_column(col)?;
Review Comment:
So does that mean `limited_convert_logical_expr_to_physical_expr_with_dfschema` will be temporary?
##########
datafusion/physical-expr-common/src/aggregate/mod.rs:
##########
@@ -495,18 +573,23 @@ impl AggregateExpr for AggregateFunctionExpr {
})
.collect::<Vec<_>>();
let mut name = self.name().to_string();
- replace_order_by_clause(&mut name);
+ // TODO: Generalize order-by clause rewrite
Review Comment:
Should we track this in a follow on ticket?
##########
datafusion/physical-expr-common/src/aggregate/mod.rs:
##########
@@ -81,6 +86,61 @@ pub fn create_aggregate_expr(
.map(|e| e.expr.data_type(schema))
.collect::<Result<Vec<_>>>()?;
+ let ordering_fields = ordering_fields(ordering_req, &ordering_types);
+ let name = name.into();
+
+ Ok(Arc::new(AggregateFunctionExpr {
+ fun: fun.clone(),
+ args: input_phy_exprs.to_vec(),
+ logical_args: input_exprs.to_vec(),
+ data_type: fun.return_type(&input_exprs_types)?,
+ name,
+ schema: schema.clone(),
+ dfschema: DFSchema::empty(),
+ sort_exprs: sort_exprs.to_vec(),
+ ordering_req: ordering_req.to_vec(),
+ ignore_nulls,
+ ordering_fields,
+ is_distinct,
+ input_type: input_exprs_types[0].clone(),
+ is_reversed,
+ }))
+}
+
+#[allow(clippy::too_many_arguments)]
+// This is not for external usage, consider creating with `create_aggregate_expr` instead.
+pub fn create_aggregate_expr_with_dfschema(
Review Comment:
Make we can also make a builder to make using this function easier (e.g. it might be easy to swap one of the last three bools by accident)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate `OrderSensitiveArrayAgg` to be a user defined aggregate [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #11564:
URL: https://github.com/apache/datafusion/pull/11564#discussion_r1687259830
##########
datafusion/expr/src/function.rs:
##########
@@ -57,6 +57,9 @@ pub struct AccumulatorArgs<'a> {
/// The schema of the input arguments
pub schema: &'a Schema,
+ /// The schema of the input arguments
Review Comment:
I hope we will have either `dfschema` or `schema` at the end 🤔
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Move `OrderSensitiveArrayAgg` to UDAF [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #11564:
URL: https://github.com/apache/datafusion/pull/11564#discussion_r1685855487
##########
datafusion/physical-expr-common/src/aggregate/mod.rs:
##########
@@ -81,6 +86,61 @@ pub fn create_aggregate_expr(
.map(|e| e.expr.data_type(schema))
.collect::<Result<Vec<_>>>()?;
+ let ordering_fields = ordering_fields(ordering_req, &ordering_types);
+ let name = name.into();
+
+ Ok(Arc::new(AggregateFunctionExpr {
+ fun: fun.clone(),
+ args: input_phy_exprs.to_vec(),
+ logical_args: input_exprs.to_vec(),
+ data_type: fun.return_type(&input_exprs_types)?,
+ name,
+ schema: schema.clone(),
+ dfschema: DFSchema::empty(),
+ sort_exprs: sort_exprs.to_vec(),
+ ordering_req: ordering_req.to_vec(),
+ ignore_nulls,
+ ordering_fields,
+ is_distinct,
+ input_type: input_exprs_types[0].clone(),
+ is_reversed,
+ }))
+}
+
+#[allow(clippy::too_many_arguments)]
+// This is not for external usage, consider creating with `create_aggregate_expr` instead.
+pub fn create_aggregate_expr_with_dfschema(
Review Comment:
first/last and ordered_array_agg are switched to this function. After #11359 , I think we can have a single `create_aggregate_expr` again.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Move `OrderSensitiveArrayAgg` to UDAF [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #11564:
URL: https://github.com/apache/datafusion/pull/11564#discussion_r1685330429
##########
datafusion/physical-expr-common/src/utils.rs:
##########
@@ -134,6 +135,33 @@ pub fn limited_convert_logical_expr_to_physical_expr(
}
}
+pub fn limited_convert_logical_expr_to_physical_expr_with_dfschema(
+ expr: &Expr,
+ dfschema: &DFSchema,
+) -> Result<Arc<dyn PhysicalExpr>> {
+ match expr {
+ Expr::Alias(Alias { expr, .. }) => Ok(
+ limited_convert_logical_expr_to_physical_expr_with_dfschema(expr, dfschema)?,
+ ),
+ Expr::Column(col) => {
+ let idx = dfschema.index_of_column(col)?;
Review Comment:
I found that I need dfschema in order to get the correct field, so I change all the UDAF to `create_aggregate_expr_with_dfschema` which is used internally for UDAF only.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Move `OrderSensitiveArrayAgg` to UDAF [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #11564:
URL: https://github.com/apache/datafusion/pull/11564#discussion_r1685845280
##########
datafusion/physical-expr-common/src/utils.rs:
##########
@@ -134,6 +135,33 @@ pub fn limited_convert_logical_expr_to_physical_expr(
}
}
+pub fn limited_convert_logical_expr_to_physical_expr_with_dfschema(
+ expr: &Expr,
+ dfschema: &DFSchema,
+) -> Result<Arc<dyn PhysicalExpr>> {
+ match expr {
+ Expr::Alias(Alias { expr, .. }) => Ok(
+ limited_convert_logical_expr_to_physical_expr_with_dfschema(expr, dfschema)?,
+ ),
+ Expr::Column(col) => {
+ let idx = dfschema.index_of_column(col)?;
Review Comment:
After removing `limited_convert_logical_expr_to_physical_expr_with_dfschema`, I think we don't need `DFSchema` anymore
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate `OrderSensitiveArrayAgg` to be a user defined aggregate [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11564:
URL: https://github.com/apache/datafusion/pull/11564#issuecomment-2244005936
Either `limited_convert_logical_expr_to_physical_expr_with_dfschema` and `limited_convert_logical_expr_to_physical_expr` are both temporary until we refactor #11359
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate `OrderSensitiveArrayAgg` to be a user defined aggregate [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #11564:
URL: https://github.com/apache/datafusion/pull/11564
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate `OrderSensitiveArrayAgg` to be a user defined aggregate [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #11564:
URL: https://github.com/apache/datafusion/pull/11564#discussion_r1687256081
##########
datafusion/physical-expr-common/src/aggregate/mod.rs:
##########
@@ -81,6 +86,61 @@ pub fn create_aggregate_expr(
.map(|e| e.expr.data_type(schema))
.collect::<Result<Vec<_>>>()?;
+ let ordering_fields = ordering_fields(ordering_req, &ordering_types);
+ let name = name.into();
+
+ Ok(Arc::new(AggregateFunctionExpr {
+ fun: fun.clone(),
+ args: input_phy_exprs.to_vec(),
+ logical_args: input_exprs.to_vec(),
+ data_type: fun.return_type(&input_exprs_types)?,
+ name,
+ schema: schema.clone(),
+ dfschema: DFSchema::empty(),
+ sort_exprs: sort_exprs.to_vec(),
+ ordering_req: ordering_req.to_vec(),
+ ignore_nulls,
+ ordering_fields,
+ is_distinct,
+ input_type: input_exprs_types[0].clone(),
+ is_reversed,
+ }))
+}
+
+#[allow(clippy::too_many_arguments)]
+// This is not for external usage, consider creating with `create_aggregate_expr` instead.
+pub fn create_aggregate_expr_with_dfschema(
Review Comment:
Great! I already file the ticket in #11543.
##########
datafusion/physical-expr-common/src/aggregate/mod.rs:
##########
@@ -81,6 +86,61 @@ pub fn create_aggregate_expr(
.map(|e| e.expr.data_type(schema))
.collect::<Result<Vec<_>>>()?;
+ let ordering_fields = ordering_fields(ordering_req, &ordering_types);
+ let name = name.into();
+
+ Ok(Arc::new(AggregateFunctionExpr {
+ fun: fun.clone(),
+ args: input_phy_exprs.to_vec(),
+ logical_args: input_exprs.to_vec(),
+ data_type: fun.return_type(&input_exprs_types)?,
+ name,
+ schema: schema.clone(),
+ dfschema: DFSchema::empty(),
+ sort_exprs: sort_exprs.to_vec(),
+ ordering_req: ordering_req.to_vec(),
+ ignore_nulls,
+ ordering_fields,
+ is_distinct,
+ input_type: input_exprs_types[0].clone(),
+ is_reversed,
+ }))
+}
+
+#[allow(clippy::too_many_arguments)]
+// This is not for external usage, consider creating with `create_aggregate_expr` instead.
+pub fn create_aggregate_expr_with_dfschema(
Review Comment:
Great! I already filed the ticket in #11543.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate `OrderSensitiveArrayAgg` to be a user defined aggregate [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11564:
URL: https://github.com/apache/datafusion/pull/11564#issuecomment-2244054205
> Thanks @alamb !
Thank you -- it is so close I can *feel* it
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate `OrderSensitiveArrayAgg` to be a user defined aggregate [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11564:
URL: https://github.com/apache/datafusion/pull/11564#issuecomment-2244011048
TODO:
1. Remove ArrayAgg Builtin definition
2. Generalize order-by clause rewrite
3. Expr Builder (not sure before the refactor or after 🤔 )
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Migrate `OrderSensitiveArrayAgg` to be a user defined aggregate [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11564:
URL: https://github.com/apache/datafusion/pull/11564#issuecomment-2244011361
Thanks @alamb !
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org