rtpsw commented on code in PR #35364:
URL: https://github.com/apache/arrow/pull/35364#discussion_r1180806897
##########
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:
I think you're right and this is mostly useful in debugging. However, note
that the schema produced by the pre-PR code is inconsistent - aggregate columns
have empty names while other column have "normal" names (derived from the
input). So, this change make sense at least for consistency.
> Does any code breaks without this change?
No. However, after adding this logic I needed to change one test case that
checks the aggregate column name. There must be a reason for the existence of
this test case, but I don't know it. @westonpace ?
--
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]
