alamb commented on code in PR #23716:
URL: https://github.com/apache/datafusion/pull/23716#discussion_r3663377381
##########
datafusion/functions-aggregate/src/array_agg.rs:
##########
@@ -827,9 +827,16 @@ impl GroupsAccumulator for ArrayAggGroupsAccumulator {
struct DistinctState {
/// Converts Arrow arrays to/from the comparable row format.
converter: RowConverter,
- /// Contiguous buffer of one encoded row per distinct value ever inserted.
- /// Dead slots (where `counts[i] == 0`) may exist after `retract_batch`.
- group_values: Rows,
+ /// One owned encoded row per live distinct value, indexed by group index.
+ /// Compacted via swap-remove on eviction so there are never dead slots.
+ group_rows: Vec<OwnedRow>,
Review Comment:
If this turns out to be too expenive (an allocation per OwnedRow) we could
also use another Rows here, though managing it will be complicated as Rows
doens't have a swap_remove function
##########
datafusion/functions-aggregate/src/array_agg.rs:
##########
@@ -817,29 +817,40 @@ impl GroupsAccumulator for ArrayAggGroupsAccumulator {
}
}
+/// Resources that are allocated lazily on the first `update_batch` call,
+/// once the concrete runtime Arrow type is known.
+///
+/// Grouping all three fields together makes the "either all present or all
+/// absent" invariant explicit in the type system, replacing the scattered
+/// `.expect()` calls that would otherwise be needed.
#[derive(Debug)]
-pub struct DistinctArrayAggAccumulator {
+struct DistinctState {
Review Comment:
👍
--
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]
