alamb commented on code in PR #23716:
URL: https://github.com/apache/datafusion/pull/23716#discussion_r3647671067
##########
datafusion/functions-aggregate/src/array_agg.rs:
##########
@@ -1546,8 +1682,10 @@ mod tests {
acc2.update_batch(&[string_list_data([vec!["e", "f", "g"]])])?;
acc1 = merge(acc1, acc2)?;
- // without compaction, the size is 16684
- assert_eq!(acc1.size(), 1684);
+ // The GroupValuesRows-based implementation uses a contiguous Rows
+ // buffer + HashTable instead of individual ScalarValue allocations,
+ // so the reported size differs from the previous implementation (was
1684).
Review Comment:
I think the context about the previous impleementation will not be relevant
after this PR merges -- it is more a comment for this PR -- just updating
without this comment I think would be less confusing
##########
datafusion/functions-aggregate/src/array_agg.rs:
##########
@@ -876,12 +970,9 @@ impl Accumulator for DistinctArrayAggAccumulator {
assert_eq_or_internal_err!(states.len(), 1, "expects single state");
- // The DISTINCT state schema is `List<value>` — partial accumulators
- // ship the set of values they saw, not multiplicities. Re-ingesting
- // each element here makes the merged counts represent "partitions
- // that emitted this value," which is fine because `evaluate` only
- // reads keys. Refcount semantics for retract are only valid within
- // a single accumulator instance (window execution).
+ // The DISTINCT state is `List<value>`. Partial accumulators ship the
+ // set of live values, not multiplicities. Re-ingesting them here is
Review Comment:
what are multiplicities? I think `DISTINCT state is List<value>` is probably
enough for this comment (i see you dodn't add this)
##########
datafusion/functions-aggregate/src/array_agg.rs:
##########
@@ -890,37 +981,38 @@ impl Accumulator for DistinctArrayAggAccumulator {
}
fn evaluate(&mut self) -> Result<ScalarValue> {
- let mut values: Vec<ScalarValue> =
self.values.keys().cloned().collect();
- if values.is_empty() {
+ if self.map.is_empty() {
return Ok(ScalarValue::new_null_list(self.datatype.clone(), true,
1));
}
- if let Some(opts) = self.sort_options {
- let mut delayed_cmp_err = Ok(());
- values.sort_by(|a, b| {
- if a.is_null() {
- return match opts.nulls_first {
- true => Ordering::Less,
- false => Ordering::Greater,
- };
- }
- if b.is_null() {
- return match opts.nulls_first {
- true => Ordering::Greater,
- false => Ordering::Less,
- };
- }
- match opts.descending {
- true => b.try_cmp(a),
- false => a.try_cmp(b),
- }
- .unwrap_or_else(|err| {
- delayed_cmp_err = Err(err);
- Ordering::Equal
- })
- });
- delayed_cmp_err?;
- };
+ let group_values = self
Review Comment:
You can likely avoid these expects / implicit state machines by using an
explicit state enum . Maybe something like this:
```rust
enum state {
Init,
Allocated {
map: ...,
group_values: ...:
converter: ...
}
}
```
--
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]
