Conversation
instead of Arel::Nodes::TableAlias. Even though Arel::Nodes::Cte and Arel::Nodes::TableAlias appear functionally equivalent here, Arel::Nodes::Cte is the semantically correct type and is more flexible for this use case, since it will allow us to add support for passing the "materialized" option to the CTE.
a63fc5d to
d648481
Compare
|
@97jaz took me a couple of attempts to get the tests to pass and the docs to look reasonable, but the PR should be ready for review now! |
while remaining fully backward compatible.
Example:
```ruby
Post.with(
Arel::Nodes::Cte.new(:post_with_comment, Post.where("comments_count > ?", 0).arel, materialized: true)
)
```
This allows for a nicer DSL for writting CTEs that is fully backward
compatible:
```ruby
Post.with(
Post.where("comments_count > ?", 0).as_cte(:post_with_comment, materialized: true),
posts_without_comments: Post.where("comments_count = ?", 0)
)
```
Also works with `with_recursive`:
```ruby
Post.with(Post.where("comments_count > ?", 0).as_cte(:post_with_comment))
Post.with(Post.where("comments_count > ?", 0).as_cte(:post_with_comment, materialized: true))
Post.with(Post.where("comments_count > ?", 0).as_cte(:post_with_comment, materialized: false))
Post.with_recursive(Post.where("comments_count > ?", 0).as_cte(:post_with_comment))
Post.with_recursive(Post.where("comments_count > ?", 0).as_cte(:post_with_comment, materialized: true))
Post.with_recursive(Post.where("comments_count > ?", 0).as_cte(:post_with_comment, materialized: false))
```
d648481 to
2b155e2
Compare
|
@Jell Thanks for doing this. In case you don't know, I don't have commit privileges in rails, so while I like this approach, I can't approve it. I do have a few comments that I'll leave inline. |
| case with_value | ||
| when Arel::Nodes::Cte then with_value | ||
| when Hash then build_with_value_from_hash(with_value) | ||
| else | ||
| raise ArgumentError, "Unsupported argument type: #{with_value} #{with_value.class}" | ||
| end |
There was a problem hiding this comment.
There are three arel node types that have #to_cte methods: As, TableAlias, and Cte (which, of course, just returns self). I wonder if we should allow any object that supports #to_cte here.
There was a problem hiding this comment.
Though, if we are going to support this, I suppose it would be better to do so in #process_with_args instead of here, so that by the time we get to this code, all of those node types would be replaced with Ctes.
There was a problem hiding this comment.
I think it would be best to do a separate PR for this if we want to support treating other types of arel nodes as valid arguments to with(). The goal of this PR is solely to find a way to have a DSL for materialized CTEs in ActiveRecord, trying to keep it as small and simple as possible.
| # Returns a Common Table Expression (CTE) from the relation to be | ||
| # used directly as input to +.with+. | ||
| def as_cte(subquery_alias, materialized: nil) | ||
| Arel::Nodes::Cte.new(subquery_alias, arel, materialized:) | ||
| end |
There was a problem hiding this comment.
Two thoughts:
- Given that the
#arelmethod (which returns anArel::SelectManager) is marked:nodoc:, I'd guess that this should be too. - I wonder if the word "arel" should be in the method name, as in
#as_arel_cteor maybe just#arel_ctefor consistency with#arel.
There was a problem hiding this comment.
@rafaelfranca are we okay with moving Arel to the public API in a case like this?
There was a problem hiding this comment.
my thinking here was that even though this returns an Arel instance, this is documented as something to pass to with, the fact that the specific type of that value is an Arel node does not make the Arel API public in my opinion.
so yes arel is not documented, but it is only used as an internal helper method here.
|
I backported this into my rails app, and have been running this in production for some time. It's really useful, as I ran into queries which the query planner was picking the wrong path, resulting in extremely slow queries. Using a materialized view solved that. @matthewd as you also commented on the original PR, maybe you can help push this (or the other PR?) along. |
|
It would be great to have this! |
Motivation / Background
This Pull Request has been created because I had the needed to add a MATERIALIZED hint to a Common Table Expression (CTE) in one of my rails applications, and that is not currently supported by the current implementation of
ActiveRecord::QueryMethods#with.Detail
The proposed change is split into 3 steps (I made a separate commit for each)
Arel::Nodes::Cteinstead ofArel::Nodes::TableAliaswhen building a CTE queryArel::Nodes::TableAliasto.with.as_ctequery method for convenienceThis change allows for CTEs to be written like this:
Additional information
I was prompted to make a pull request with this proposal in on this thread: #48305 (comment)
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]