icexelloss commented on code in PR #35364:
URL: https://github.com/apache/arrow/pull/35364#discussion_r1180823651
##########
cpp/src/arrow/engine/substrait/options.cc:
##########
@@ -205,12 +205,17 @@ class DefaultExtensionProvider : public
BaseExtensionProvider {
ARROW_ASSIGN_OR_RAISE(auto aggregate, internal::ParseAggregateMeasure(
agg_measure, ext_set,
conv_opts,
/*is_hash=*/!keys.empty(),
input_schema));
+ aggregate.name = aggregate.function;
Review Comment:
@rtpsw I am happy to merge the code that fixes the the segmented aggregation
bug (didn't pass the segmented key). and the change that addes validation of
number of output names However for the internal aggregation names change we
would need validate that this naming is consistent with other aggregation
codepath and/or refactor the naming logic to be shared (e.g., move that logic
into ParseAggregateMeasure), so there is more work. I would suggest revert that
change for now so we can move forward with the bug fix faster. (Since the
naming change seems cosmetic)
--
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]
