-
Notifications
You must be signed in to change notification settings - Fork 96
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
Split traversal component from DataflowToSqlQueryPlanConverter
#1517
Conversation
822d119
to
e264be5
Compare
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.
Left one recommendation about naming!
@@ -1956,3 +1923,38 @@ def visit_window_reaggregation_node(self, node: WindowReaggregationNode) -> SqlD | |||
group_bys=outer_query_select_columns, | |||
), | |||
) | |||
|
|||
@staticmethod | |||
def _make_time_range_comparison_expr( |
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 encapsulating this in a better way 🙏
) | ||
|
||
|
||
class DataflowNodeToSqlSubqueryVisitor(DataflowPlanNodeVisitor[SqlDataSet]): |
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.
I'm not sure about the name here - it feels like removing Subquery
would be more accurate (i.e., DataflowNodeToSqlVisitor
), since there isn't always a subquery involved. Technically, each node resolves to a full SQL query (and the parent node often resolves to a subquery).
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.
Maybe DataflowNodeToDefaultSqlVisitor
? Issue with DataflowNodeToSqlVisitor
is differentiating this one from the new one that uses CTEs. I'll have to put renames at the end due to stack conflicts though.
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.
Works for me!
This PR splits off the part that does the dataflow traversal in
DataflowToSqlQueryPlanConverter
into a separate class to reduce the class size. Later, these two parts can be put into different files.