-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
refactor: split expression pf planner into one part. #4783
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jackwener -- this looks great. 🙏 the planner is in desperate need of some cleaning up
Something else that would help I think is to move the tests from datafusion/sql/src/planner.rs
into datafusion/sql/test/planner.rs
or similar to help parallelize the test compilation
Sadly I have just created a bunch of conflicts in in #4779 (also related to shrinking the planner)
I make this PR draft, I will wait for those PR merge. |
Thank YOU! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,
i would merge this pr first, otherwise there will be conflicts.
Benchmark runs are scheduled for baseline = 00a22cb and contender = 6c81c10. 6c81c10 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Thank you for this! |
Which issue does this PR close?
Part of #4392.
I'm try into add
Binder
in planner, split the planner is a good begin for it. So I do it previously.Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?