-
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
Extract logical plans in LogicalPlan as independent struct: TableScan #1290
Conversation
I extracted I have two questions:
|
@xudong963, If you split them into multiple tasks, you can list each subtask, may be I can help you to do someone. |
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 for the contribution @xudong963 - this PR looks exactly like what is discussed on #1228 👍
I think this looks like an improvement to me. It will be a fairly large, but mechanical change to the codebase but I do think it will help over time
What do you think about this change @jimexist @houqp @rdettai and @Dandandan
My pleasure! I believe after finishing this, it'll bring help for subqueries implementation. |
CreateMemoryTable
, DropTable
, CreateExternalTable
in LogicalPlan as independent struct
#1306
The ticket can be merged? Then we can push related tickets🎯 |
Any concerns @Dandandan / @andygrove / @jimexist / @rdettai ? |
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.
Thanks for your work @xudong963. I am not sure I fully understand the big picture of the change though. It seems from #1228 that this is part of adding subqueries, but I would find it clearer if there was an umbrella issue with bullet points that list to steps/issues.
I have also raised some questions here: #1228 (comment)
There is some good discussion on #1228 (comment) -- to be clear, in my mind, the purpose of this refactoring is to make Thanks @xudong963 for leading the charge |
Which issue does this PR close?
Related to #1228
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?