Convert bool_and & bool_or to UDAF
#11009
Conversation
github-actions
bot
added
logical-expr
| Accumulator, AggregateUDFImpl, GroupsAccumulator, ReversedUDAF, Signature, Volatility, | ||
| }; | ||
|
|
||
| use datafusion_physical_expr_common::aggregate::groups_accumulator::bool_op::BooleanGroupsAccumulator; |
There was a problem hiding this comment.
I could use some help here on how to proceed with extracting BooleanGroupsAccumulator from physical-expr-common.
The BooleanGroupsAccumulator depends on NullState which in turn has other users as seen here:
$ rg NullState -c
datafusion-examples/examples/advanced_udaf.rs:4
datafusion/physical-expr/src/lib.rs:1
datafusion/physical-expr/src/aggregate/average.rs:3
datafusion/physical-expr/src/aggregate/groups_accumulator/mod.rs:2
datafusion/physical-expr-common/src/aggregate/groups_accumulator/bool_op.rs:4
datafusion/physical-expr-common/src/aggregate/groups_accumulator/prim_op.rs:4
datafusion/physical-expr-common/src/aggregate/groups_accumulator/accumulate.rs:15There was a problem hiding this comment.
It's also imported by the following in functions-aggregate:
countbit_and_or_xorsum
So maybe this is better tackled in a separate PR and is ok for now? 🤔
There was a problem hiding this comment.
I think we should leave BooleanGroupsAccumulator in physical-expr-common until we have moved the other boolean aggregate functionss over -- then I think BooleanGroupsAccumulator should be able to move without issue
| } | ||
|
|
||
| fn order_sensitivity(&self) -> AggregateOrderSensitivity { | ||
| AggregateOrderSensitivity::Insensitive |
There was a problem hiding this comment.
The default is AggregateOrderSensitivity::HardRequirement. Is the use of Insensitive here for bool_and and bool_or the correct usage?
There was a problem hiding this comment.
Insensitive makes sense to me -- @mustafasrepo perhaps you can confirm?
| bool_and(lit(true)), | ||
| bool_or(lit(true)), |
There was a problem hiding this comment.
Added to roundtrip_expr_api.
There was a problem hiding this comment.
Thank you @jcsherin -- this is a really neat first PR
I think we should remove the commented out tests, but then this looks good to go from my perspective
cc @jayzhan211
| Accumulator, AggregateUDFImpl, GroupsAccumulator, ReversedUDAF, Signature, Volatility, | ||
| }; | ||
|
|
||
| use datafusion_physical_expr_common::aggregate::groups_accumulator::bool_op::BooleanGroupsAccumulator; |
There was a problem hiding this comment.
I think we should leave BooleanGroupsAccumulator in physical-expr-common until we have moved the other boolean aggregate functionss over -- then I think BooleanGroupsAccumulator should be able to move without issue
| } | ||
|
|
||
| fn order_sensitivity(&self) -> AggregateOrderSensitivity { | ||
| AggregateOrderSensitivity::Insensitive |
There was a problem hiding this comment.
Insensitive makes sense to me -- @mustafasrepo perhaps you can confirm?
|
I've pushed the following changes based on the code review:
|
There was a problem hiding this comment.
Thank you @jcsherin -- looks great to me. I'll wait a while to merge this to let @jayzhan211 / @mustafasrepo have a chance to review if they want
|
Thanks again @jcsherin and @jayzhan211 |
* Port `bool_and` and `bool_or` to `AggregateUDFImpl` * Remove trait methods with default implementation * Add `bool_or_udaf` * Register `bool_and` and `bool_or` * Remove from `physical-expr` * Add expressions to logical plan roundtrip test * minor: remove methods with default implementation * Removes redundant tests * Removes hard-coded function names
* Port `bool_and` and `bool_or` to `AggregateUDFImpl` * Remove trait methods with default implementation * Add `bool_or_udaf` * Register `bool_and` and `bool_or` * Remove from `physical-expr` * Add expressions to logical plan roundtrip test * minor: remove methods with default implementation * Removes redundant tests * Removes hard-coded function names
* Port `bool_and` and `bool_or` to `AggregateUDFImpl` * Remove trait methods with default implementation * Add `bool_or_udaf` * Register `bool_and` and `bool_or` * Remove from `physical-expr` * Add expressions to logical plan roundtrip test * minor: remove methods with default implementation * Removes redundant tests * Removes hard-coded function names
3 participants
Which issue does this PR close?
Closes #11008.
What changes are included in this PR?
bool_and,bool_orto UDAFExprsroundtrip_expr_apitestAre these changes tested?
aggregate.sltAre there any user-facing changes?