getChan commented on code in PR #24814:
URL: https://github.com/apache/datafusion/pull/24814#discussion_r3894749047
##########
datafusion/functions-aggregate/src/min_max/min_max_struct.rs:
##########
@@ -83,21 +83,11 @@ impl GroupsAccumulator for MinMaxStructAccumulator {
matches!(partial_cmp_struct(a, b), Some(Ordering::Greater))
}
- if self.is_min {
- self.inner.update_batch(
- array.as_struct(),
- group_indices,
- total_num_groups,
- struct_min,
- )
- } else {
- self.inner.update_batch(
- array.as_struct(),
- group_indices,
- total_num_groups,
- struct_max,
- )
- }
+ let cmp: fn(&StructArray, &StructArray) -> bool =
+ if self.is_min { struct_min } else { struct_max };
+
+ self.inner
+ .update_batch(array.as_struct(), group_indices, total_num_groups,
cmp)
}
fn evaluate(&mut self, emit_to: EmitTo) -> Result<ArrayRef> {
Review Comment:
nit: This refactoring coerces `struct_min` / `struct_max` to a function
pointer. Since `cmp` is called from the per-row loop in
`MinMaxStructState::update_batch`, this is a hot path where even small overhead
can accumulate.
I compared the release LLVM IR locally: the updated version selects a
function pointer and invokes it indirectly inside the loop, whereas the
original version produces separate MIN/MAX loops.
The original code is a little more verbose, but would you mind keeping the
two branches here to preserve static dispatch?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
