Dandandan commented on code in PR #21794:
URL: https://github.com/apache/datafusion/pull/21794#discussion_r3128944562
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/bytes_view.rs:
##########
@@ -166,8 +166,18 @@ impl<B: ByteViewType> ByteViewGroupValueBuilder<B> {
Nulls::None => {
self.nulls.append_n(rows.len(), false);
- for &row in rows {
- self.do_append_val_inner(arr, row);
+ if arr.data_buffers().is_empty() {
+ // Fast path: all strings are inline (≤12 bytes).
+ // The input array's u128 views are already in the correct
format;
+ // copy them directly instead of going through value() →
make_view().
+ self.views.extend(rows.iter().map(|&row|
arr.views()[row]));
+ } else {
+ // Slow path: some strings are non-inline (>12 bytes).
+ // Pre-reserve views capacity to avoid repeated
reallocation.
+ self.views.reserve(rows.len());
+ for &row in rows {
+ self.do_append_val_inner(arr, row);
Review Comment:
In principle we can make this faster as well - `extend` + reuse input view
(instead of make_view) + avoid `array.value(row)`.
Up to you - can be part of follow-up PR
--
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]
