mustafasrepo commented on code in PR #4675:
URL: https://github.com/apache/arrow-datafusion/pull/4675#discussion_r1053713402
##########
datafusion/physical-expr/src/aggregate/min_max.rs:
##########
@@ -541,22 +543,38 @@ pub fn max_row(index: usize, accessor: &mut RowAccessor,
s: &ScalarValue) -> Res
#[derive(Debug)]
pub struct MaxAccumulator {
max: ScalarValue,
+ moving_max: moving_min_max::MovingMax<ScalarValue>,
}
impl MaxAccumulator {
/// new max accumulator
pub fn try_new(datatype: &DataType) -> Result<Self> {
Ok(Self {
max: ScalarValue::try_from(datatype)?,
+ moving_max: moving_min_max::MovingMax::<ScalarValue>::new(),
})
}
}
impl Accumulator for MaxAccumulator {
fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
- let values = &values[0];
- let delta = &max_batch(values)?;
- self.max = max(&self.max, delta)?;
+ for idx in 0..values[0].len() {
Review Comment:
Thanks for the review @alamb, as in our previous disccussion when @Ted-Jiang
proposed an algorithm. We have updated our design to address your concern. For
expressions that do not require sliding window implementations(such your
example) we use `ForwardAggregateWindowExpr`. This implementation uses more
optimized implementation (since it doesn't assume retract). Currently optimized
algorithms are supported for accumulators having `row_accumulator_supported`
boolean `true`.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
