mustafasrepo commented on code in PR #6034:
URL: https://github.com/apache/arrow-datafusion/pull/6034#discussion_r1175568710
##########
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>. To support dynamic calculation of the
output ordering. See the corresponding
[synnada-ai#78](https://github.com/synnada-ai/arrow-datafusion/pull/78) for
more information.
>
> The linked PR [synnada-ai#78
(comment)](https://github.com/synnada-ai/arrow-datafusion/pull/78#issue-1657633081)
says that the issue is related to the AggregateExec changing during
optimization, which I don't understand -- if the input changes then a new
AggregateExec is also created
As you say, this approach may be unnecessary. Initially I thought,
information during the construction of the `AggregateExec` may change during
planning, this would make us produce sub-optimal plans. However, when input
changes new `AggregateExec` is also created, and we have all the information
necessary. I will discuss it with our team, whether we lose any functionality
with old API( probably we are not). After that discussion I will do the
necessary changes. Thanks for pointing it out
--
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]
