alamb commented on code in PR #21794:
URL: https://github.com/apache/datafusion/pull/21794#discussion_r3364933570
##########
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:
as a possible future optimization, we could use `get_unchecked` here if it
makes any difference
##########
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:
Also, from here on down I think this is basically the same as
append_val_inner -- if there are any differences perhaps we can fold it into
append_val_iner and avoid the copy.
Is this something you can look into as a follow on PR @EeshanBembi ?
--
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]
