-
Notifications
You must be signed in to change notification settings - Fork 1k
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
move functionality out of PhysicalPlanBuilder into other classes and add tests #405
Conversation
@dguy you have moved the physical layer functionality into the logical layer. We want to keep logical plan phase independent of the physical plan phase. |
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.
You have moved the physical layer functionality into the logical layer. We want to keep logical plan phase independent of the physical plan phase.
@hjafarpour, not sure I follow your point. I understand that the However, I have been looking at the physical plan builder code, and it really is quite complicated. In particular, the recursive calls into While this does mix the strictly logical The proposed structure makes things a bit easier to read and reason about. More importantly, when in the future when we add more types of Is it really that important that the logical and physical layers are separated in the previous way? What are the advantages of that approach in your mind? |
@apurvam yes, keeping logical plan independent of physical plan layer is essential since in logical layer we should not care about how we implement the plan in destination physical layer. Our default physical layer is Kafka Streams at the moment but in future we will also consider other execution contexts such as connect or 3rd party systems. So it is essential to keep any dependency to the underlying physical plan away and keep logical plan layer abstract. |
I (obviously) agree with @apurvam. This makes the code easier to reason about. The logic is actually in the classes it needs to be rather than all dumped in the |
I totally agree with the readability and testability argument but we would be violating a more important principal here which is keeping logical layer independent of physical layer. I would suggest breaking up the PhysicalPlanBuilder by creating new classes in the same layer instead of spilling the physical implementation code in the abstraction layer. All data management systems have kept their logical planning code independent of implementation details and we should not violate this principal. |
As it is in this PR if we add new extensions of PlanNode we only have to add 1 class that encapsulates the behaviour. If we do it another way we need add multiple classes every time we change something. IMO, this is not a good design. If we put the logic with the data we can just add a single class and it works. |
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.
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.
leggit
No description provided.