alamb commented on code in PR #6034:
URL: https://github.com/apache/arrow-datafusion/pull/6034#discussion_r1175324013
##########
datafusion-examples/examples/custom_datasource.rs:
##########
@@ -217,7 +217,7 @@ impl ExecutionPlan for CustomExec {
datafusion::physical_plan::Partitioning::UnknownPartitioning(1)
}
- fn output_ordering(&self) -> Option<&[PhysicalSortExpr]> {
+ fn output_ordering(&self) -> Option<Vec<PhysicalSortExpr>> {
Review Comment:
~Why does this signature need to change? The new signature requires an
allocation / `clone` of a Vec where the previous one didn't and thus this seems
to change the API for the worse.~
Update: this is explained in the PR description:
> This functionality requires us to calculate output_ordering dynamically.
For this reason, this PR accompanies the api change from fn
output_ordering(&self) -> Option<&[PhysicalSortExpr]> to fn
output_ordering(&self) -> Option<Vec<PhysicalSortExpr>>. To support dynamic
calculation of the output ordering. See the corresponding
https://github.com/synnada-ai/arrow-datafusion/pull/78 for more information.
Perhaps we can calculate the output ordering once in the constructor rather
than on demand.
```
fn output_ordering(&self) -> Option<Vec<PhysicalSortExpr>> {
self.calc_aggregation_ordering().map(|state| state.ordering)
}
```
--
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]
