Adds Arel::Nodes::Cte for use in WITH expressions#48261
Merged
tenderlove merged 1 commit intorails:mainfrom May 24, 2023
Merged
Adds Arel::Nodes::Cte for use in WITH expressions#48261tenderlove merged 1 commit intorails:mainfrom
tenderlove merged 1 commit intorails:mainfrom
Conversation
tenderlove
approved these changes
May 24, 2023
Member
tenderlove
left a comment
There was a problem hiding this comment.
@97jaz do you mind fixing the conflicts and I'll merge? This PR looks good to me.
SelectManager#with currently accepts As and TableAlias nodes. Neither of these support materialization hints for the query planner. Both Postgres and SQLite support such hints. This commit adds a Cte node that does support materialization hints. It continues to support As and TableAlias nodes by translating them into Cte nodes.
Contributor
Author
|
@tenderlove Done -- thanks! |
| @@ -1,3 +1,26 @@ | |||
| * Add `Arel::Nodes::Cte` for use in `WITH` expressions | |||
Member
There was a problem hiding this comment.
Should this be in the CHANGELOG? Arel is private so this isn't really a feature we should be promoting in the CHANGELOG since it is in theory not user facing.
Contributor
Author
There was a problem hiding this comment.
Ah -- I didn't consider that. I can open a PR to remove the changelog entry.
Member
|
Thanks! This was just what I had in mind. Sorry, I initially left it to allow time for other opinions, then I forgot to come back. 😔 |
97jaz
added a commit
to 97jaz/rails
that referenced
this pull request
May 26, 2023
Following on the Arel work in rails#48261, this commit adds the ability to provide a materialization hint to a CTE in ActiveRecord. To support the hint, a more verbose syntax for CTE values is introduced: Post.with(t: { query: Post.where("comments_count > ?", 0), materialized: true }) The CTE value is described as a hash that contains a required :query key and an optional :materialized key. The set of available options could be expanded in the future to accomodate, for example, postgres's various options for recursive CTEs.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation / Background
Postgres and SQLite both support a non-standard extension to the CTE syntax to indicate that a CTE subquery should be materialized, i.e., not folded into the main query but evaluated separately. This can be useful in cases where the query planner would otherwise make poor decisions.
The syntax, in both databases, is:
WITH foo AS MATERIALIZED (...) ...Similarly, they support an explicit hint that a CTE should not be materialized:
WITH foo AS NOT MATERIALIZED (...) ...Detail
This Pull Request adds
Arel::Nodes::Cte, which represents a CTE query and contains an alias, a relation, and a materialization flag. The latter is eithertrueto request materialization,falseto request no materialization, ornilwhich omits any materialization hint. The node can be used like so:AsandTableAliasnodes continue to be supported as arguments toSelectManager#with.Additional information
This is an alternative to #47951, following @matthewd's suggestion of adding a
Ctenode that contains both an alias and a relation, instead of adding a node just to represent the addition of theMATERIALIZEDkeyword. This keeps the syntax tree shallower and allows us easily to supportNOT MATERIALIZEDhints without needing to resort to yet another keyword-specific node type or deeper nesting of nodes.Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]