-
Notifications
You must be signed in to change notification settings - Fork 178
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
Use CTE #656
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.
I like the simplicity of this approach and the very small size of the patch. The query_list
argument is a bit "magical" -- its type determines if CTEs are used.
Could sql_render()
return an intermediate data structure that is identical for the CTE and non-CTE case? Perhaps a query container that combines raw SQL, subqueries, and literal values. The final composition would then either embed the subqueries or use them as CTEs, and also either embed the literals inline or use placeholders and query parameters.
Yes, it is a bit magical... But this was the easiest way I came up with to combine few changes and no breaking changes.
Strictly speaking this would be a breaking change but it should not affect much code. This would also be useful for further optimising the generated SQL code (e.g. using
I'm not sure I fully understand what you mean here. Can you give an example? |
I implemented CTEs using the new lazy render pipeline in this PR #729. It looks way nicer than this approach. |
Closed in favour of #790. |
Closes #638.
I hacked together the support for CTE. Adding CTEs to
dbplyr
definitely seems doable. Together with custom join aliases this would greatly improve readability.There might be some limitations where CTEs cannot be used or where this implementation doesn't work. Therefore, I added a parameter
cte
so the user can decide whether to use CTE clauses or not. I'd propose to not use CTEs by default (at least for now) and see whether things work fine. It might be worth to use an option as default value.Some notes
sql_render()
due to the "branching" in join and set op queries.sql_render()
forselect_query
,set_op_query
,join_query
,semi_join_query
we need to register the subqueries and give them names. This is currently done via passing a list.cte = TRUE
. It would probably be nicer to passcte
explicitly but I had the feeling that quite a lot of functions would need to know about it and pass it along. I'll check again later if this is doable or not.Example for select query
Example for join query
Reuse CTE