-
Notifications
You must be signed in to change notification settings - Fork 7
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
Splitting a query into CTEs #21 #45
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.
Thanks for this PR. It's useful.
I would advise taking another look at the implementation. For me, the current implementation is too adhoc, and doesn't fit in well with the rest of the code. I think the code can be much more elegant by pre-splitting each query into CTEs, and making View
objects with each CTE.
lea/runner.py
Outdated
view = self.views[view_key].with_context( | ||
table_reference_mapping=table_reference_mapping | ||
) | ||
if materialize_ctes and isinstance(view, SQLView): | ||
print(self._materialize_ctes_and_view) | ||
job = functools.partial(self._materialize_ctes_and_view, view=view) | ||
else: | ||
job = functools.partial( | ||
self.client.materialize_view, | ||
view=view, | ||
) |
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 wouldn't approach it this way. What I would suggest, is to split the query into CTEs, and create a View
object for each CTE. I would then add these objects to the list of views that will be executed. This way, you benefit from the rest of lea, and don't need to handle the execution adhoc. I think you should rework your code so that it leverages what already exists, if that makes sense.
Related to #21
Parsing SQL queries and extraction of CTEs within queries.
Add --materialize-ctes option in the CLI to activate this feature.
Materializing CTEs so we can analyze them quickly.
I hope this helps you at carbonfact @MaxHalford 🤞🤞
pytest -s test_examples.py::test_jaffle_shop_materialize_ctes