leung-ming commented on code in PR #1893:
URL: https://github.com/apache/datafusion-comet/pull/1893#discussion_r2179708541
##########
native/spark-expr/src/agg_funcs/avg_decimal.rs:
##########
@@ -341,29 +337,16 @@ impl AvgDecimalGroupsAccumulator {
}
}
- fn is_overflow(&self, index: usize) -> bool {
- !self.is_empty.get_bit(index) && !self.is_not_null.get_bit(index)
- }
-
+ #[inline]
fn update_single(&mut self, group_index: usize, value: i128) {
- if self.is_overflow(group_index) {
- // This means there's a overflow in decimal, so we will just skip
the rest
- // of the computation
- return;
- }
-
- self.is_empty.set_bit(group_index, false);
let (new_sum, is_overflow) =
self.sums[group_index].overflowing_add(value);
self.counts[group_index] += 1;
+ self.sums[group_index] = new_sum;
Review Comment:
The final sum now only depend on the original sum and the argument `value`,
no need to check it is overflow or not.
1. No need to branching or conditional move, same above.
2. Less registers are required(no need to keep the original sum and
`new_sum` before checking if it is overflow or not), which may further increase
the instruction parallelism on modern CPU, with the hoping that the outer loop
is unrolled automatically.
3. Less work branched, less work to revert when branch predictor miss.
--
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...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
