EeshanBembi commented on code in PR #21794:
URL: https://github.com/apache/datafusion/pull/21794#discussion_r3365168024
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/bytes_view.rs:
##########
@@ -166,8 +170,50 @@ 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).
+ // Read views directly to avoid array.value(row) overhead
and
+ // reuse the source view's prefix instead of recomputing
it via make_view.
+ self.views.try_reserve(rows.len()).map_err(|e| {
+ datafusion_common::exec_datafusion_err!(
+ "failed to reserve {0} views: {e}",
+ rows.len()
+ )
+ })?;
+ for &row in rows {
+ let view = arr.views()[row];
Review Comment:
Sure, I can look into both of these in a follow-up PR: trying
`get_unchecked` for the view access and refactoring the slow path to reuse
`append_val_inner` (or consolidate the duplicated logic). Thanks for the review!
--
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]
