Skip to content
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

Adds Arel::Nodes::Cte for use in WITH expressions #48261

Merged
merged 1 commit into from May 24, 2023

Conversation

97jaz
Copy link
Contributor

@97jaz 97jaz commented May 19, 2023

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 either true to request materialization, false to request no materialization, or nil which omits any materialization hint. The node can be used like so:

    posts = Arel::Table.new(:posts)
    comments = Arel::Table.new(:comments)

    good_comments_query = comments.project(Arel.star).where(comments[:rating].gt(7))
    cte = Arel::Nodes::Cte.new(:good_comments, good_comments_query, materialized: true)

    query = posts.
      project(Arel.star).
      with(cte).
      where(posts[:id].in(cte.to_table.project(:post_id))).

    puts query.to_sql

    # WITH "good_comments" AS MATERIALIZED (SELECT * FROM "comments" WHERE "comments"."rating" > 7) SELECT * FROM "posts" WHERE "posts"."id" IN (SELECT post_id FROM "good_comments")

As and TableAlias nodes continue to be supported as arguments to SelectManager#with.

Additional information

This is an alternative to #47951, following @matthewd's suggestion of adding a Cte node that contains both an alias and a relation, instead of adding a node just to represent the addition of the MATERIALIZED keyword. This keeps the syntax tree shallower and allows us easily to support NOT MATERIALIZED hints 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:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

Copy link
Member

@tenderlove tenderlove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.
@97jaz
Copy link
Contributor Author

97jaz commented May 24, 2023

@tenderlove Done -- thanks!

@tenderlove tenderlove merged commit 2dbc7db into rails:main May 24, 2023
8 of 9 checks passed
@@ -1,3 +1,26 @@
* Add `Arel::Nodes::Cte` for use in `WITH` expressions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah -- I didn't consider that. I can open a PR to remove the changelog entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthewd
Copy link
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants