zeroshade commented on issue #1296:
URL: https://github.com/apache/arrow-go/issues/1296#issuecomment-5765547326
Thanks for writing this up—the overall direction looks right. A dedicated
scalar-aggregate function/executor, the `Init`/`Consume`/`Merge`/`Finalize`
lifecycle, exact dispatch, preserving scalar spans, finalizing empty input,
keeping aggregates out of `exprs`, and the two-PR split all make sense.
On the two questions:
1. I would not use `scalar.Scalar` as the result type for the exported
general `ScalarAggKernel`. It is sufficient for the eight functions in this
proposal, including `min_max` as a struct scalar, but C++ uses `Datum`
deliberately: `tdigest` is a `ScalarAggregateFunction` whose finalizer returns
an array. Since `exec` cannot import `compute`, please add a small `exec`-owned
aggregate result carrier that can hold either an owned scalar or owned
`arrow.ArrayData`, and let the compute executor box that into a `Datum`. The
ownership rule should be explicit: `Finalize` returns one owned result, and
executor cleanup must not invalidate it.
2. Keep `SkipNulls` as a plain bool with the C++ name and `compute` tag. Nil
options should select `DefaultScalarAggregateOptions()` with `{SkipNulls: true,
MinCount: 1}`; an explicitly supplied zero value should remain `{false, 0}`.
`MinCount` already means the zero value cannot equal the default, and
`TakeOptions` is good precedent for using an explicit default helper rather
than inverting the field.
A few framework details I would include before exporting this API:
- Add aggregate-state cleanup and ensure it runs exactly once on success,
empty input, cancellation, and init/consume/merge/finalize errors.
- Add `Ordered bool` now, defaulting to false, and document `Merge`
precisely: `dst` is mutated, `src` and `dst` cannot alias, source ownership
after merge, and ordered partitions merge in logical input order.
- Add `TypeName` methods, option-deserialization registration, and
`DefaultCountOptions` if those were only omitted from the note.
- Prefer `CountAllRows` or `CountModeAll` over `CountAll`, so the package
name remains available for a future `count_all` convenience function.
- Add a framework ownership test with a buffer-backed or nested result, plus
a scalar-span test where `Len > 1`; primitive `count`/`sum` tests will not
expose those lifecycle bugs.
The kernel semantics and test matrix otherwise look well scoped for the
numeric/boolean first pass. With those interface changes, please push the
framework + `count`/`sum` PR.
--
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]
