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

Common Table Expression support added "out-of-the-box" #37944

Merged
merged 1 commit into from Jul 5, 2022

Conversation

vlado
Copy link
Contributor

@vlado vlado commented Dec 12, 2019

Summary

This PR adds .with query method which makes it super easy to build complex queries with Common Table Expressions.

It basically just "wraps" the passed arguments in the way Arel::SelectManager.with expects it. The biggest advantages this brings are:

  • You can just use standard ActiveRecord relations instead of manually building Arel::Nodes::As nodes
  • It returns ActiveRecord::Relation so you don't loose any flexibility

See example below:

Post.with(
  posts_with_comments: Post.where("comments_count > ?", 0),
  posts_with_tags: Post.where("tags_count > ?", 0)
)

# Replaces

posts_with_comments_table = Arel::Table.new(:posts_with_comments)
posts_with_comments_expression = Post.arel_table.where(posts_with_comments_table[:comments_count].gt(0))
posts_with_tags_table = Arel::Table.new(:posts_with_tags)
posts_with_tags_expression = Post.arel_table.where(posts_with_tags_table[:tags_count].gt(0))

Post.all.arel.with([
  Arel::Nodes::As.new(posts_with_comments_table, posts_with_comments_expression),
  Arel::Nodes::As.new(posts_with_tags_table, posts_with_tags_expression)
])
# WITH posts_with_comments AS (
#   SELECT * FROM posts WHERE (comments_count > 0)
# ), posts_with_tags AS (
#   SELECT * FROM posts WHERE (tags_count > 0)
# )
# SELECT * FROM posts

@kaspth
Copy link
Contributor

kaspth commented Dec 15, 2019

@vlado @simi looks like you're doing the same work at the same time. Ref: #37973

Maybe you want to compare notes as well?

@simi
Copy link
Contributor

simi commented Dec 15, 2019

@kaspth not sure how could I miss that. I have pushed also fix for update_all/delete_all in my branch and it is missing recursive for now (to keep the PR simple).

@vlado would you mind if I port some of your changes into my PR, mark you as an author and continue in there? You have really nice documentation and also I can port some tests.

@vlado
Copy link
Contributor Author

vlado commented Dec 15, 2019

Hi @simi and @kaspth thanks for the feedback. What a coincidence that we worked on same thing same time :)

I forgot to mention in PR description that I already added support for recursive expressions Post.with(:recursive, cool_posts: "union to get cool posts..."). It follows Arel API for recursive calls. See more here: https://github.com/rails/rails/pull/37944/files#diff-25e9a621b650cd62a407c4ce7a117b20R77-R86.

I agree that we co-author this but I think it is easier to finish it here. From what I see only thing missing is update_all / delete_all support?

@kaspth What do you think?

@simi
Copy link
Contributor

simi commented Dec 15, 2019

@vlado the recursive api looks a little weird for me. I don't think there's any other AR relation method which uses "argument as a modifier of behaviour". Therefore I have decided in my PR to not introduce it for now to not block basic implementation and open another PR later once the first one get merged.

Comparing our code changes I have spotted those:

  1. I have separated with logic to separate class (ActiveRecord::With). Would you mind to port them as well if you like that?

  2. In build_arel method you have removed support for Array and for Hash your code is different. Can you explain on those changes? My idea was to not change logic/API to get 100% compatible with original ctes_in_my_pg gem to get easy transition.

Finally if you can prepare failing specs for delete_all and update_all I can submit PR to your branch to support those relation calls as well.

@vlado
Copy link
Contributor Author

vlado commented Dec 15, 2019

Hi @simi, thanks for the constructive feedback. Replies are below.

@vlado the recursive api looks a little weird for me. I don't think there's any other AR relation method which uses "argument as a modifier of behaviour". Therefore I have decided in my PR to not introduce it for now to not block basic implementation and open another PR later once the first one get merged.

Hm, you could be right here. I also can not think of any AR relation method at this moment. I initially started with with.recursive(...) but when I checked how Arel works I saw that .with(:recursive, ...) is used and I liked it cause it is much simpler to implement. I was also thinking about .with_recursive.

Comparing our code changes I have spotted those:

  1. I have separated with logic to separate class (ActiveRecord::With). Would you mind to port them as well if you like that?

Looks like a good idea 👍

  1. In build_arel method you have removed support for Array and for Hash your code is different. Can you explain on those changes? My idea was to not change logic/API to get 100% compatible with original ctes_in_my_pg gem to get easy transition.

Arrays are still supported but only if all items are Arel::Nodes::As. I didn't add documentation for this cause I didn't see options where you can pass Arel nodes documented anywhere (probably because Arel API is considered private).

Regarding Hash changes, I think there was an issue with select = Arel::Nodes::SqlLiteral.new("(#{expression.to_sql})") line in one of the tests in SQLite (I can check it later). Also, I tried to add meaningful error messages if unsupported arguments are passed.

I never used https://github.com/DavyJonesLocker/postgres_ext or https://github.com/kmurph73/ctes_in_my_pg in any of my apps (i was using my own patch) but I used them as inspiration when starting this PR.
That being said, I think I haven't break any compatibility with them in case of the upgrade, and even if we break some it should be super easy to upgrade. I think we should prioritise to make this best it can be.

Finally if you can prepare failing specs for delete_all and update_all I can submit PR to your branch to support those relation calls as well.

Sure, I'll add failing tests.

@kaspth
Copy link
Contributor

kaspth commented Dec 15, 2019

Post.with(
  posts_with_comments: Post.where("comments_count > ?", 0),
  posts_with_tags: Post.where("tags_count > ?", 0)
)

I'm not too keen on this API. Seems verbose, yet still fairly low level. I haven't used CTEs before but just realized a case where I could, what's the use cases you've needed them for? And how do you act on the values? I'm guessing posts_with_comments/posts_with_tags are available to use in a custom SQL string?

What I'm saying is since Rails is an extracted, not built, framework, I'd prefer to see full production samples (with renames or some business logic stripped if needed). It seems like with is only half the equation, but there's also selecting on the resulting CTE. Plus what people can pass to with also includes a UNION ALL, I'd be curious to see those cases too. All in the name of figuring out how to cut and where to scope the API. Active Record isn't necessarily just Arel but nicer, it's free to be more geared. 😊

@simi
Copy link
Contributor

simi commented Dec 15, 2019

@kaspth CTEs are super useful since they work as a "temporary table". Our codebase is full of CTEs and this will make it much nicer, since we usually need to fallback to just executing SQL string written in heredoc. Also queries using CTE are often more readable. This will allow to compose those queries based on AR scopes instead of messing with SQL strings (we usually interpolate SQL string with relation.to_sql).

Yes, I agree all examples in this (and my PR) are really simple and not real reason to use CTE. I think there are articles around describing CTE and how/when to use them. On the other side Rails active record tests are full of examples not really making sense, but doing great job testing the feature.

I understand this not really well known feature of SQL (mainly for beginners). I have learned about that just few months ago. Introducing this feature with nice and friendly guide explaning purpose of this Active Record API will be great service to help people understanding CTEs.

What I'm saying is since Rails is an extracted, not built, framework, I'd prefer to see full production samples (with renames or some business logic stripped if needed).

Just one quick example I do remember is we need to group and filter data and then join them with belongs_to relation. I'll try to prepare some examples with current code and how it will work with current pull request to show you some real examples extracted from real application.

ℹ️ This is also super useful in combination with recently introduced bulk upserts in combination with RETURNING (which did not make it yet to Rails codebase).

@vlado
Copy link
Contributor Author

vlado commented Dec 15, 2019

Great questions @kaspth and you are guessing right :)

I'll try to explain with one example

Example

At BetterDoc we have a 60+ lines SQL query that we use to dynamically calculate clinic rank based on some params (medical problem, geo location and radius, ...).
Keep in mind that this is a super simplified version that should be enough to explain the use case.

Domain

We have 2 models in our domain. Clinic with name only, and Case Number which stores number of operations (count) clinic did for specific medical topic (code).

Screenshot 2019-12-15 20 43 02

class Clinic
  has_many :case_numbers
end

class CaseNumber
  belongs_to :clinic
end

SQL Query

Here is a full SQL that is explained below.

WITH relevant_clinics AS (
    SELECT clinics.id, clinics.name, SUM(case_numbers.count) AS cases_count 
    FROM clinics INNER JOIN case_numbers ON case_numbers.clinic_id = clinics.id
    WHERE case_numbers.code ILIKE 'F4%'
    GROUP BY clinics.id 
),
stats AS (
    SELECT
        PERCENTILE_CONT(0.25) WITHIN GROUP (ORDER BY cases_count DESC) AS quartile_2,
        PERCENTILE_CONT(0.50) WITHIN GROUP (ORDER BY cases_count DESC) AS quartile_1
    FROM relevant_clinics
)

SELECT name, cases_count,
    CASE 
        WHEN cases_count > (SELECT quartile_2 FROM stats) THEN 'High'
        WHEN cases_count > (SELECT quartile_1 FROM stats) THEN 'Average'
        ELSE 'Low' 
    END AS rank
FROM relevant_clinics
ORDER BY cases_count DESC

First step is to get clinics that match provided medical topic and store them as temporary result set (relevant_clinics). Then we reference them to get some statistics from them and save those as another temporary results set (stats). We then fire main query which references both of them to get final results with rank and other stuff we need.

How to organise this in Rails

Since ActiveRecord does not have support for CTE we started thinking how to organise this in our app.

We started by putting SQL into heredoc and executing it with find_by_sql and count_by_sql. This could work for some report or similar but it didn't work for us cause we needed ActiveRecord::Relation flexibility (chainability, lazy load, ...).

Next try was with Arel.with and it was a step forward but we were still missing ActiveRecord::Relation goodies. Clinic.where(Clinic.arel_table[:id].in(arel_that_returns_clinic_ids)) "hack" was not an option cause we needed to keep order and other stuff from original query.

In the end I wrote a monkey patch that adds .with to ActiveRecord::Relation and we were able to write our query like this.

class Clinic < ApplicationRecord
  has_many :case_numbers

  scope :matches_medical_topic, -> (q) { joins(:case_numbers).where("case_numbers.code ILIKE ? "#{q}%") }

  def self.search(q)
    relevant_clinics = matches_medical_topic(q)
      .select("cllinics.id, clinics.name, sum(case_numbers.count) AS cases_count
      .group("clinics.id")

    stats = select(
        "PERCENTILE_CONT(0.25) WITHIN GROUP (ORDER BY cases_count DESC) AS quartile_2",
        "PERCENTILE_CONT(0.50) WITHIN GROUP (ORDER BY cases_count DESC) AS quartile_1"
      )
      .from("relevant_clinics")

    with(relevant_clinics: relevant_clinics, stats: stats)
      .from("relevant_clinics AS clinics")
      .order("cases_count DESC")
  end

  def self.with_rank
    select(:id, :name, :cases_count)
      .select("CASE WHEN cases_count > (SELECT quartile_2 FROM stats) THEN 'High' WHEN cases_count > (SELECT quartile_1 FROM stats) THEN 'Average' ELSE 'Low' END as rank")
  end
end

and use it like:

Clinic.search("f4").with_rank # => ActiveRecord::Relation
Clinic.search("f4").count # => Integer

Since there are gems to do same thing I assumed that other people would like to see this in Rails so I started this PR.

Conclusion

You can see from my example that I used from("relevant_clinics AS clinics") to be able to chain it with other relations. Alternative is to use join joins("JOIN relevant_clinics ON relevant_clinics.id = clinics.id").

I absolutely agree that this is a bit too verbose and low level but I haven't been able to figure out better API yet. I'm eager to hear your ideas and to make it more "geared" :)

Maybe the way to go could would be to have something similar to associations and/or scopes that once defined will abstract everything for the user. Similar to the way joins(:association_name)works. Option to go low level (current implementation) could also stay.

class Post
  has_many :case_numbers

  common_table :relevant_clinics, -> (q) { join(:case_numbers).where("code = ?", q) }
  common_table :stats, -> { select(...).from(:relevant_clinics) }
end

Post.with(:relevant_clinics, :stats)

@vlado
Copy link
Contributor Author

vlado commented Dec 15, 2019

I see now that I forgot about UNION example. You can find one in tests here https://github.com/rails/rails/pull/37944/files#diff-25e9a621b650cd62a407c4ce7a117b20R77-R86

Not a real life example but it is getting late here in Croatia :)

@simi
Copy link
Contributor

simi commented Dec 16, 2019

@vlado thanks for very detailed info. Our usecase and story is really similar, but in the end I have used https://github.com/kmurph73/ctes_in_my_pg as a Relation#with implementation.

@kaspth my idea was to introduce this with logic into AR step by step. Start with simple with, then continue with recursive if it will be needed. We can also later see how with is doing and if there are some new patterns like that "scope-like" common_table idea mentioned in @vlado 's comment.

I think demand for WITH support is not neglible. You can google for cte rails active record site:stackoverflow.com (sorry, I have no idea how to share search result link). There are also some blog post mentioning how to integrate CTEs into ActiveRecord.

Few examples including which strategy is used to integrate CTE to ActiveRecord (just selected from first search page):

PS: I was thinking about gemifying this as well in some general way (not supporting only PostgreSQL), but monkeypatching of private methods from ActiveRecord and Arel would be needed. That was the main motivation to send my pull request to integrate this into core.

@vlado
Copy link
Contributor Author

vlado commented Dec 16, 2019

@vlado thanks for very detailed info. Our usecase and story is really similar, but in the end I have used https://github.com/kmurph73/ctes_in_my_pg as a Relation#with implementation.

I found out about ctes_in_my_pg late, postgres_ext was not working with Rails 6 so I did my own patch.

I think demand for WITH support is not neglible.

Totally agree. We are using CTEs with Rails for more then 4 years now and it is a pain from the beginning. I was also giving talks about this topic (organising complex SQL queries in Rails) few times and from the feedback regarding CTE I was actually surprised that this was not implemented yet and decided to do it properly this time :)

@kaspth
Copy link
Contributor

kaspth commented Dec 22, 2019

@kaspth my idea was to introduce this with logic into AR step by step. Start with simple with, then continue with recursive if it will be needed. We can also later see how with is doing and if there are some new patterns like that "scope-like" common_table idea mentioned in @vlado 's comment.

That's pretty much the opposite of what I'm curious to explore here or how I'd proceed. I'd rather we have as many real world cases to base this off of and extract the commonalities. Then if that looks like something that's worth it, we can consider adding that and some lower level constructs. As it is I don't think there's enough meat here to add with and I'm thinking of bowing out and passing to someone else on the team who feels more strongly about this.

For instance the common_table abstraction doesn't have much sway with me currently, from what I've gathered CTEs are for one off queries, so I'd personally rather put all those parts together.

I think demand for WITH support is not neglible. You can google for cte rails active record site:stackoverflow.com (sorry, I have no idea how to share search result link). There are also some blog post mentioning how to integrate CTEs into ActiveRecord.

Sure, unfortunately that doesn't necessarily mean that we want to maintain something in Active Record directly. Then we'd be looking at APIs for gems instead, of which the current proposed with seems to fit with better.

@vlado I'm curious about your usage of from in your examples, can you expand on that? Particularly where you alias the relevant_clinics into clinics?

Another thing that's tough for me, although I like the with_rank split, is that cases_count isn't defined in there. So calling with_rank is dependent on the other method first.

@simi
Copy link
Contributor

simi commented Dec 23, 2019

@kaspth just try to take a look at this feature from different perspective.WITH is just another SQL clause, same as WHERE, FROM, JOIN or ORDER for example. WITH was introduced in SQL-99 standard and is widely supported by all "major" relation databases.

Common table expressions are supported by Teradata, DB2, Firebird,[8] Microsoft SQL Server, Oracle (with recursion since 11g release 2), PostgreSQL (since 8.4), MariaDB (since 10.2), MySQL (since 8.0), SQLite (since 3.8.3), HyperSQL and H2 (experimental).[9] Oracle calls CTEs "subquery factoring".[10]

https://en.wikipedia.org/wiki/Hierarchical_and_recursive_queries_in_SQL#Common_table_expression

My motivation to support WITH in ActiveRecord is to make it possible to compose queries using WITH (for any reason) via nice ActiveRecord chainable API. The same you can do with custom joins, selects, wheres and orders today.

Sometimes you can avoid SQL oriented approach and just ask ActiveRecord for data like Account.all.eager_load(:users) and let the ActiveRecord do the best to build the query for you. Sometimes you need to use more SQL oriented approach and build chain of few custom selects, joins, wheres, ... and get the desired result. IMHO both approaches are totally valid. ActiveRecord excels in both of them already. Introducing WITH support will make the second approach even more flexible.

Another thing that's tough for me, although I like the with_rank split, is that cases_count isn't defined in there. So calling with_rank is dependent on the other method first.

Yes, but I think this is something you need to handle carefully already if you use more SQL oriented approach. For example if you select or order by relation column, you need to ensure related table is properly joined.

Sure, unfortunately that doesn't necessarily mean that we want to maintain something in Active Record directly. Then we'd be looking at APIs for gems instead, of which the current proposed with seems to fit with better.

Supporting this in gem is really hard since ActiveRecord internals are changing often and this needs to monkey patch various lower level APIs including AREL ones. There's solid support in AREL already for WITH. I'll extract my AREL change to make this work on update_all/delete_all as well to separated pull request (not introducing Relation#with, but just fixing this in AREL for UpdateManager and DeleteManager). Having this change merged will make it at least a little more easier to maintain in gemified form and AREL can benefit from that as well.

For instance the common_table abstraction doesn't have much sway with me currently, from what I've gathered CTEs are for one off queries, so I'd personally rather put all those parts together.

As it is I don't think there's enough meat here to add with and I'm thinking of bowing out and passing to someone else on the team who feels more strongly about this.

CTEs are not definitely useful just for one off queries. We use them for regular queries for various reasons already mentioned before. If you would like to not continue in this review I would like to thank you anyway for your time spent already on this. I can try to bring some attention here to get someone another to review if this feature is not actually finally rejected by that decision.

PS: Originally I have forgotten to mention that it is probably possible (I'm not sure if that will be possible for all adapters) to fix some problems using WITH internally. For example my work on #35638 lacks support for "through" associations. Using WITH can fix that by creating associations in WITH part, return associated ids and make entries in "linking" table in second using ids returned from CTE. That's also one of the usecases we use. Pipe queries together and use output of one as a input to another one without need to send data from DB to app and back.

@vlado
Copy link
Contributor Author

vlado commented Jan 6, 2020

@vlado I'm curious about your usage of from in your examples, can you expand on that Particularly where you alias the relevant_clinics into clinics?

@kaspth I'm aliasing relevant_clinics as clinics here to be able to easily chain and call other query methods later. For example to do something like:

Clinic.search("f4").with_rank.where(id: [1, 2, 3])
# WITH relevant_clinics AS (...) SELECT ... FROM relevant_clinics clinics WHERE clinics.id IN (1,2,3)

As mentioned you could also use join joins("JOIN relevant_clinics ON relevant_clinics.id = clinics.id")

Another thing that's tough for me, although I like the with_rank split, is that cases_count isn't defined in there. So calling with_rank is dependent on the other method first.

Agreed, I also did not liked it but it was good enough :)

Really appreciate your participation on this PR and would not want you to quit. You asked great questions and I like your thinking.

I would say that biggest challenge (at least it was for us) is that you have expression(s) that builds your temporary table(s) and then you have multiple ways you want to use them (select without rank, select with rank, aggregate, call some additional where, ...) which makes it hard to put all this things together in one abstraction. That is way I proposed the common_table abstraction which is responsible just for the first part (half the equation as you said) and then you are responsible to do something with it. Similar how joins works CaseNumber.joins(:clinic).where("clinics.name ILIKE ?", "%query%"). Not saying this is perfect solution but I'm more then happy to explore this further.

That being said I must also agree with @simi that just adding with method to Relation is already a huge improvement and it makes using CTE's with Rails 100 times better :)
Also, even when we find a proper abstraction and API for common cases with should keep current functionality cause there will always be a case when you need to go low level to build your query.

PS. I'm sorry for a late reply. Was more focused on family stuff for holidays ☃️

@rails-bot
Copy link

rails-bot bot commented Jan 12, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Jan 12, 2022
@mildred
Copy link

mildred commented Jan 18, 2022

What's missing to get this merged?

Having CTE available in rails would be a major improvement.

@rails-bot rails-bot bot removed the stale label Jan 18, 2022
@rails-bot
Copy link

rails-bot bot commented Apr 18, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Apr 18, 2022
@simi
Copy link
Contributor

simi commented May 21, 2022

@vlado are you still interested in this PR? Would you mind rebasing?

# # )
# # SELECT comments.* FROM replies
#
# or pass them as a `Hash` but be aware that `ActiveRecord::Relation` does not have `UNION` support
Copy link
Contributor

@simi simi May 21, 2022

Choose a reason for hiding this comment

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

I was thinking about introducing UNION support ActiveRecord, this could be great reasoning.

Copy link
Contributor

@casperisfine casperisfine left a comment

I wonder if @kamipo has opinions about this feature.

activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
activerecord/lib/active_record/relation/query_methods.rb Outdated Show resolved Hide resolved
activerecord/lib/active_record/relation/query_methods.rb Outdated Show resolved Hide resolved
activerecord/lib/active_record/relation/query_methods.rb Outdated Show resolved Hide resolved
# # )
# # SELECT * FROM posts_with_tags AS posts
#
# Post.with(:posts_with_tags, Post.where("tags_count > ?", 0).joins("JOIN posts_with_tags ON posts_with_tags.id = posts.id"))
Copy link
Contributor

@kbrock kbrock May 24, 2022

Choose a reason for hiding this comment

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

I think the parens on this are wrong. Maybe:

Suggested change
# Post.with(:posts_with_tags, Post.where("tags_count > ?", 0).joins("JOIN posts_with_tags ON posts_with_tags.id = posts.id"))
# Post.with(:posts_with_tags, Post.where("tags_count > ?", 0)).joins("JOIN posts_with_tags ON posts_with_tags.id = posts.id")

B. It is too bad we have to resort to this joins clause.
I say this because it feels like I will want to join the primary model to the CTE for most (all?) of my uses of CTE. Not sure if it is the same for you.

Is it possible to implement:

Post.with(:posts_with_tags, Post.where("tags_count > ?", 0)).joins(:posts_with_tags)

Copy link
Contributor Author

@vlado vlado Jun 26, 2022

Choose a reason for hiding this comment

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

I think it is possible and I proposed some ideas already #37944 (comment) but as suggested I wouldn't implement it here to keep this PR small and easy to grasp.

Simple scopes like this could be enough for start maybe.

@vlado
Copy link
Contributor Author

vlado commented May 26, 2022

Glad to see interest again 🕺, will try to address everything this weekend.

@simi
Copy link
Contributor

simi commented May 30, 2022

Btw. CTE can be used internally to batch upsert data (even for has_many through relations). I can try to dive into that once merged.

raise ArgumentError, "Bulk insert or upsert is currently not supported for has_many through association"

@vlado
Copy link
Contributor Author

vlado commented Jun 26, 2022

@casperisfine @kbrock @simi I've followed your feedback and made following changes:

  • Removed all mentions of Arel API from Changelogs and docs.
  • Removed support for recursive queries.
  • Only supports hash as an argument now. (It is also possible to pass already prepared Arel::Nodes::As or Arel::Nodes::TableAlias as arguments but not mentioned in the documentation. Not sure if we should remove them also, they could be useful for internal use?)

This is now a minimal implementation to make CTE in Rails easy to use (pass them as AR::Relation and get AR::Relationback) and to build on in the future.

byroot
byroot approved these changes Jun 30, 2022
Copy link
Member

@byroot byroot left a comment

Thanks for reducing the scope. It looks good to me now.

@byroot byroot added the ready PRs ready to merge label Jun 30, 2022
@byroot
Copy link
Member

byroot commented Jun 30, 2022

Could you rebase? There's lots of unrelated CI failures because of the minitest update. I'll leave a bit of time for other maintainers to chime in if they want, but I don't see any objections to merging.

hash.map do |name, value|
expression =
case value
when Arel::Nodes::SqlLiteral then Arel.sql("(#{value})")
Copy link
Member

@matthewd matthewd Jun 30, 2022

Choose a reason for hiding this comment

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

Use a Grouping node to add parens, don't re-create the SqlLiteral by modifying its string form.

else
raise ArgumentError, "Unsupported argument type: `#{value}` #{value.class}"
end
Arel::Nodes::TableAlias.new(expression, Arel.sql(name.to_s))
Copy link
Member

@matthewd matthewd Jun 30, 2022

Choose a reason for hiding this comment

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

I don't know the specific node this should be off the top of my head, but it definitely should not inject the hash key as raw SQL; name should end up table/column quoted.

Copy link
Contributor Author

@vlado vlado Jun 30, 2022

Choose a reason for hiding this comment

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

@matthewd Thanks for catching those! Both fixed in 5b8676c


with_statements = with_values.map do |with_value|
case with_value
when Arel::Nodes::As, Arel::Nodes::TableAlias then with_value
Copy link
Member

@matthewd matthewd Jun 30, 2022

Choose a reason for hiding this comment

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

I don't mind keeping this if e.g. your planned use case really works a lot better if it's allowed, but I don't think we should retain undocumented extra argument options just because it might be internally useful later: if it is, we can add it then; adding it early makes life harder later because if it exists, someone will call it.

Copy link
Contributor Author

@vlado vlado Jun 30, 2022

Choose a reason for hiding this comment

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

Agreed, will remove it.

Copy link
Contributor Author

@vlado vlado Jun 30, 2022

Choose a reason for hiding this comment

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

Removed in 464d5ac

Copy link
Contributor

@casperisfine casperisfine Jun 30, 2022

Choose a reason for hiding this comment

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

Removed in 464d5ac

Ah forgot to mention, you'll have to squash all these 24 commits into a single one.

Copy link
Contributor Author

@vlado vlado Jun 30, 2022

Choose a reason for hiding this comment

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

Ah forgot to mention, you'll have to squash all these 24 commits into a single one.

Sure. Just addressed feedback from @matthewd and will squash if all is ok.

Copy link
Contributor Author

@vlado vlado Jul 1, 2022

Choose a reason for hiding this comment

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

@casperisfine Squashed and tests are green.

@casperisfine
Copy link
Contributor

casperisfine commented Jun 30, 2022

Also the CI failures look legit now.

Construct common table expressions with ease and get `ActiveRecord::Relation` back.
@byroot byroot merged commit b3175b9 into rails:main Jul 5, 2022
4 checks passed
@byroot
Copy link
Member

byroot commented Jul 5, 2022

Thank you @vlado. Feel free to open a second PR for recursive support.

@vlado
Copy link
Contributor Author

vlado commented Jul 5, 2022

Thank you @vlado. Feel free to open a second PR for recursive support.

Great, thanks @byroot!

paracycle added a commit to Shopify/tapioca that referenced this issue Aug 10, 2022
paracycle added a commit to Shopify/tapioca that referenced this issue Aug 10, 2022
paracycle added a commit to Shopify/tapioca that referenced this issue Aug 10, 2022
paracycle added a commit to Shopify/tapioca that referenced this issue Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
activerecord ready PRs ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet