You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/08/24 14:41:03 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #7396: Make `LogicalPlan::with_new_exprs,` deprecate `from_plan`
alamb opened a new pull request, #7396:
URL: https://github.com/apache/arrow-datafusion/pull/7396
## Which issue does this PR close?
related to https://github.com/apache/arrow-datafusion/pull/7359
## Rationale for this change
While reiewing a great documentation improvement from @tshauck it took me a while to find `from_plan` used to rewrite LogicalPlans. See https://github.com/apache/arrow-datafusion/pull/7359#discussion_r1304408534
I think this makes it harder to work with the DataFusion codebase unecessairly
The reason it took me a while to find I think was
1. Isn't method on `LogicalPlan` (it is in a utility module).
2. It didn't have `expr` or `exprs` in its name
Thus, given it takes a `&LogicalPlan` as its first input I think it makes sense and would be more easily findable as a method on `LogicalPlan`.
## What changes are included in this PR?
1. Move code to create new `LogicalPlan`s with rewrtitten apis to `LogicalPlan::with_new_exprs()`
4. deprecate `from_plan`
3. I also noticed it often would force a copy of Expr's twice given its API so I made `LogicalPlan::new_with_exprs` take a `Vec<Expr>` instead of `&[Expr]` as it needs them owned anyways
## Are these changes tested?
By existing tests
## Are there any user-facing changes?
New API, deprecated old one
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #7396: Make `LogicalPlan::with_new_exprs,` deprecate `from_plan`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7396:
URL: https://github.com/apache/arrow-datafusion/pull/7396#issuecomment-1706939066
@liukun4515 / @jackwener could I trouble you for a review of this PR? I can no longer merge such PRs without an approval of another committer.
Thank you, Andrew
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #7396: Make `LogicalPlan::with_new_exprs,` deprecate `from_plan`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7396:
URL: https://github.com/apache/arrow-datafusion/pull/7396
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org