Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate TreeNode transform_xx_mut methods #10097

Closed
alamb opened this issue Apr 16, 2024 · 2 comments · Fixed by #10126
Closed

Deprecate TreeNode transform_xx_mut methods #10097

alamb opened this issue Apr 16, 2024 · 2 comments · Fixed by #10126
Labels
enhancement New feature or request

Comments

@alamb
Copy link
Contributor

alamb commented Apr 16, 2024

Is your feature request related to a problem or challenge?

Part of #8913

While working on the documentation in #10035, @peter-toth made the great observation #10035 (comment) that:

I have no idea why we have the ..._mut() version, most likely for just convenience, but IMO one transform_down() with FnMut should be fair enough.

Specifically, these methods are basically the same, except that _mut takes a mutable reference to the closure

  • transform_down / transform_down_mut
  • transform_up / transform_up_mut

The current API is pretty confusing enough already (see discussion on #10035) so having redundant methods just makes it more confusing

Also, this terminology leaked into the LogicalPlan variants transform_down_with_subqueries_mut etc in #9913

Describe the solution you'd like

I would like to deprecate

  • transform_down_mut
  • transform_up_mut

And remove the (yet unreleased) logical plan variants transform_down_with_subqueries_mut and transform_up_with_subqueries_mut

Describe alternatives you've considered

No response

Additional context

No response

@alamb alamb added the enhancement New feature or request label Apr 16, 2024
@peter-toth
Copy link
Contributor

I'm happy to take this and file a PR.

@alamb
Copy link
Contributor Author

alamb commented Apr 16, 2024

Thanks @peter-toth 🙏

I had a short play with it as well and it may be slightly tricker to sort out than I had imagined. However, I am sure you'll figure it out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants