westonpace commented on code in PR #14867:
URL: https://github.com/apache/arrow/pull/14867#discussion_r1042308712
##########
python/pyarrow/_compute.pyx:
##########
@@ -2211,11 +2213,18 @@ def _group_by(args, keys, aggregations):
c_aggregations.push_back(c_aggr)
with nogil:
- result = GetResultValue(
+ c_agg_batches = GetResultValue(
GroupBy(c_args, c_keys, c_aggregations)
)
- return wrap_datum(result)
+ result_batches = []
+ for c_batch in c_agg_batches:
+ result_batch = []
+ for c_column in c_batch.values:
+ result_batch.append(wrap_datum(c_column))
+ result_batches.append(result_batch)
Review Comment:
Yes, I was a bit torn on this one. The simplest thing might be for
`arrow::compute::GroupBy` to return `std::shared_ptr<RecordBatch>`. However, I
don't have column names, so I would be making those up. Also, the inputs are
datums, and so it seemed like a mismatch to receive datums (not arrays) and
return a record batch (and not an exec batch). So then I ended up with a list
of lists of arrays which is unpleasant too.
I could return a list of record batches but then I would have to copy the
`CreateSimpleSchema` method which invents names for these columns. Since the
caller of this function has those names, and this function is private, I
figured it best to leave that work for the caller.
That being said, after sleeping on this a bit, maybe a better change would
be to change `arrow::compute::GroupBy` to receive arrays (not datums) and then
returning a record batch wouldn't be inconsistent.
--
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]
