Make union/except/intersect chainable as SelectManager #320
Conversation
|
I also have a version of this against 5-0-stable here. |
| @@ -447,7 +452,7 @@ def visit_Arel_Nodes_Grouping o, collector | |||
| end | |||
|
|
|||
| def visit_Arel_SelectManager o, collector | |||
| collector << "(#{o.to_sql.rstrip})" | |||
| collector << "( #{o.to_sql.rstrip} )" | |||
There was a problem hiding this comment.
I think just to make the spacing of inner queries consistent with outer queries, unless @ofavre has a better answer for why he originally did it that way.
|
I'd love to see this merged. Seems like every year or so I encounter a "needs union" use case, and instead have to leave a tangle of complex Arel code that no one else understands. |
|
It is frustrating AREL doesn't support this, there are a lot of use cases for chaining union operators. It would be great if this was resolved. |
|
@amalagaura: Yeah, I rely on this behavior and keep needing to update this branch to work with new versions. If the maintainers have no interest in merging this functionality, perhaps I should instead figure out a way to do it with a monkey-patch or a separate library. @rafaelfranca / @tenderlove / other maintainers: I'm not sure if you've seen this, but a thumbs-up or thumbs-down on this functionality would be really nice, so that I have a sense for whether or not I'm wasting my time on it. If the functionality is desired, but you don't like the implementation, I'm willing to put together a different PR that speaks to that. |
|
This seems like a no-brainer. I also have 3 Arel::SelectManagers that I need to merge and I am forced to do a .to_sql and join(' UNION '). Arel needs union to not return a Node::Binary and instead return a proper SelectManager, otherwise union really is broken since SQL definitely supports chainable unions. |
|
I'd also like to see a better way to work with unions. Feedback on @jsanders pull request please? |
|
How has this been ignored a year? @tenderlove ?? |
|
I sincerely apologize for not having time for this. Can you please rebase it and I will review. I'm really sorry. |
|
@tenderlove: No worries! I'll find some time to rebase and ping you again. |
|
@jsanders Did you rebase? We'd love to have this. Thanks! |
Fixes rails#118 Fixes rails#98 Conflicts: lib/arel/nodes/and.rb lib/arel/nodes/select_statement.rb lib/arel/visitors/mssql.rb lib/arel/visitors/to_sql.rb Conflicts: lib/arel/nodes/and.rb lib/arel/nodes/binary.rb lib/arel/predications.rb lib/arel/select_manager.rb lib/arel/visitors/depth_first.rb lib/arel/visitors/mssql.rb lib/arel/visitors/to_sql.rb test/visitors/test_mysql.rb test/visitors/test_oracle.rb test/visitors/test_to_sql.rb
It doesn't make sense and any uses are almost certainly buggy.
Except is an nary node, so the visitor must use `inject_join` rather than `infix_value`.
jsanders
force-pushed
the
master-with-union-fix
branch
from
6a4e516
to
32f9081
Compare
|
@tenderlove: Rebased this onto current master and things look pretty good. Feedback welcome! Sorry for the delay, folks. |
|
@tenderlove Waiting on your tender PR love. Thanks. |
|
@tenderlove Any updates? I have another use case and don't want to use |
|
Hello. What about this? Arel::Nodes::Union.new(select_manager_1, Arel::Nodes::Union.new(select_manager_2, select_manager_3))This gives us "chained" unions. Some over-parenthesized but workable solution. We can generalize it a'lil: def build_union(left, right)
if right.length > 1
Arel::Nodes::UnionAll.new(left, build_union(right[0], right[1..-1]))
else
Arel::Nodes::UnionAll.new(left, right[0])
end
end
managers = [select_manager_1, select_manager_2, select_manager_3]
build_union(managers[0], managers[1..-1]).to_sql
# => ( (SELECT table1.* from table1)
# UNION ALL
# ( (SELECT table2.* from table2)
# UNION ALL
# (SELECT table3.* from table3) ) )This works in Arel 4.0.2... No need to |
|
Per #523, Arel development is moving to rails/rails. If this PR is still relevant, please consider reopening it over there. |
Fixes #118.
This is the same as #239, but against master instead of 4-0-stable. It would be really nice to get this in so we don't need to keep freshening it! Or if there's a reason it isn't being merged, feedback would be great.