[C++] Simplify AggregateNodeOptions aggregates/targets

[asfimport]

Currently AggregateNodeOptions is:

class ARROW_EXPORT AggregateNodeOptions : public ExecNodeOptions {
 public:
  // aggregations which will be applied to the targetted fields
  std::vector<internal::Aggregate> aggregates;
  // fields to which aggregations will be applied
  std::vector<FieldRef> targets;
  // output field names for aggregations
  std::vector<std::string> names;
  // keys by which aggregations will be grouped
  std::vector<FieldRef> keys;
};

It is not very obvious how aggregates and targets are related. My initial read of the comments led me to think that each aggregate would be applied to each target and you would end up with len(aggregates) * len(targets) output fields. In reality the aggregate at index i only applies to the target at index i. It would be simpler to add a FieldRef target to internal::Aggregate (and Aggregate should not be internal).

Alternatively, the entire internal::Aggregate could be replaced by a "call" arrow::compute::Expression

Reporter: Weston Pace / @westonpace
Assignee: Vibhatha Lakmal Abeykoon / @vibhatha

PRs and other links:

• GitHub Pull Request #13150

_{Note: This issue was originally created as ARROW-16549. Please see the migration documentation for further details.}

[asfimport]

Vibhatha Lakmal Abeykoon / @vibhatha:
cc @westonpace  I think this make sense. Also it is easier for the user. I think it would be okay to have it as a field within the Aggregate and put it in arrow::compute:: namespace as you're suggesting. But call is much straight forward. 

But to make it clear to the user, I would say the struct is fine for now. WDYT?

[asfimport]

Vibhatha Lakmal Abeykoon / @vibhatha:
How about including names field within the aggregates? cc @westonpace 

I have added the main change which is moving 'targetinside theAggregate` object. 

But the current change breaks the R-bindings. And looking at this specific piece of code, 

arrow/r/R/query-engine.R

Line 125 in c032290

node <- node$Aggregate(

I think, may be we can put the names inside the Aggregate? (optional or user-provided).

Appreiciate your thoughts,   @nealrichardson

[asfimport]

Weston Pace / @westonpace:
Moving names inside of the Aggregate makes a lot of sense to me. I think that's a good idea.

[asfimport]

Weston Pace / @westonpace:
Issue resolved by pull request 13150
#13150