zeroshade commented on code in PR #1338:
URL: https://github.com/apache/arrow-go/pull/1338#discussion_r4169799482
##########
arrow/compute/exec/aggregate.go:
##########
@@ -184,6 +184,11 @@ type ScalarAggKernel struct {
// for merging partitions in the logical order of the input. Kernels
// whose result does not depend on the order of the input leave this
// false.
+ //
+ // The executor in this package consumes the whole input into a single
Review Comment:
The executor isn't in this package (`exec`). `scalarAggExecutor` lives in
`compute`. Suggest "The executor in the compute package…", which matches the
`AggregateResult` doc.
##########
arrow/compute/exec/aggregate.go:
##########
@@ -250,6 +255,11 @@ var _ AggKernel = (*ScalarAggKernel)(nil)
// not the merge succeeded; on an error the first state is cleaned up as well
// and the returned state is nil, so that the caller never has to clean up a
// state MergeAll was given.
+//
+// With no states there is nothing to merge or to finalize, and MergeAll
+// returns (nil, nil). A caller that ended up with zero partitions aggregates
+// the empty input the way the executor does: create one state with the init
+// function and finalize it.
Review Comment:
Suggest "…and finalize it, then clean it up." The paragraph above says
callers never clean up states passed to `MergeAll`, but this state is the
caller's to clean up.
--
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]
