kosiew commented on code in PR #24070:
URL: https://github.com/apache/datafusion/pull/24070#discussion_r3709520619
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/row_backed.rs:
##########
@@ -425,6 +429,84 @@ mod tests {
assert_eq!(g0, 20);
}
+ /// `take_n` preallocates the retained-row buffer from the known row
Review Comment:
The test does a good job of confirming that rebuilding works correctly, but
I do not think it distinguishes the new preallocation logic from the previous
`empty_rows(0, 0)` path. The outer-null `FixedSizeList` has a shorter row
encoding, but that row is emitted rather than retained. The retained inner-null
and non-null `Int32` rows appear to have the same encoded length.
Could we change or add retained rows with different encoded sizes, perhaps
using `FixedSizeList<Utf8>` values with unequal payload lengths? That would
exercise the `lengths().skip(n).sum()` preallocation calculation more directly.
--
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]
