-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement retract_batch for AvgAccumulator #4846
Conversation
Add avg to custom window frame tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for the contribution!
let delta = sum_batch(values, &self.sum.get_datatype())?; | ||
self.sum = self.sum.sub(&delta)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like giving a name to the change (i.e. delta
). I suggest doing the same in update_batch
.
@mustafasrepo, PTAL |
Thank you for contribution @jonmmease and for the review @ozankabak ❤️ |
LGTM thanks @jonmmease. |
Can this be merged? Thanks! |
Sorry for the delay @jonmmease -- thanks again for the contributions! |
Benchmark runs are scheduled for baseline = 42f7dd5 and contender = 13fb42e. 13fb42e is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #4845.
What changes are included in this PR?
Adds an implementation of
retract_batch
toAvgAccumulator
that is identical to the implementation forSumAccumulator
.Are these changes tested?
I added
AVG
to a couple of existing tests that previously failed with"Retractable Accumulator hasn't been implemented..."
error.Are there any user-facing changes?
AVG
can now be used in window expressions with custom frames.