mustafasrepo commented on code in PR #4675:
URL: https://github.com/apache/arrow-datafusion/pull/4675#discussion_r1054078775
##########
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:
I missed this point sorry, while answering I had in mind a query such as
`MIN(c12) OVER (ORDER BY C12 RANGE BETWEEN UNBOUNDED PRECEDING AND 0.2
FOLLOWING`. This query was running with optimized implementation, however, your
example was running with sliding implementation(with overhead). I sent a commit
to fix this issue. Now your example should be running a non-sliding
implementation. Thanks for pointing it out. I have followed your suggestion to
solve this issue (Name of the new accumulators are `SlidingMinAccumulator` and
`SlidingMaxAccumulator`).
--
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
