Skip to content

Chainable unions #118

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.

Sign up for GitHub

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

Jump to bottom
Closed
jroes wants to merge 2 commits into from
Closed

Chainable unions #118

jroes wants to merge 2 commits into from

Conversation

@jroes
Copy link

@jroes jroes commented Apr 13, 2012

I'm working on a solution to the chainable unions problem. See also #98

I ended up making Union a Unary node. I could see a case for it being binary, but the solution seemed simpler this way.

I think I'm pretty close here, but I've got a single failing test for WITH RECURSIVE. I believe this is because it expects that a union will always be enclosed within parentheses, but I don't know if that's necessarily valid.

Let me know if I'm going down the right path?

@jroes
Copy link
Author

jroes commented Apr 13, 2012

I also just got this rocking with ActiveRelation over at jroes/rails@06fec10. If this looks good, I could potentially port the code over to master and make a pull request to rails as well.

@ofavre
Copy link

ofavre commented Aug 3, 2012

I tried to tackle that same problem, along with creating a union method on Relation, I implemented with the same idea.

I don't think of Union as a Unary node: unions can have its own LIMIT clause, just like its subqueries:

(SELECT * FROM table1 LIMIT 10) UNION (SELECT * FROM table2 LIMIT 10) LIMIT 10

The previous query would merge top results from table1 first, then add non duplicates top results from table2, then limit the overall results to 10.
In such case, parenthesis are mandatory, to avoid confusion (the limit clause of the last select would have been attached to union), and syntax errors.

I rather think of Unions as a toplevel construct, having multiple subqueries, rather that what you made, ie. a select query having multiple subqueries to link to, which prevents the first query from having its own LIMIT (et al) clauses.

ofavre pushed a commit to yakaz/rails that referenced this pull request Aug 3, 2012
ActiveRecord::Relation unions [PROPOSAL], closes rails#939
3d032b2 
Depends on rails/arel#118.
The accepted implementation could act as
a unions list, or a new top-level object,
which would change this patch proposal.
ofavre pushed a commit to yakaz/rails that referenced this pull request Aug 3, 2012
ActiveRecord::Relation unions [PROPOSAL], closes rails#939
29b8ebd 
Depends on rails/arel#118.
The accepted implementation could act as
a unions list, or a new top-level object,
which would change this patch proposal.
@ablignaut
Copy link

ablignaut commented Feb 15, 2013

Hi Jonathan

Do you know when /if this pull request will be merged?

@jroes
Copy link
Author

jroes commented Feb 26, 2013

I think @ofavre has a better handle on this than I do, my vote is to go his direction.

@jroes jroes closed this Feb 26, 2013
ofavre pushed a commit to yakaz/arel that referenced this pull request Feb 26, 2013
WIP on rails#118
00033b2 
As Work In Progress promises, there is no guarantees on its working
status and minimality.

I actually worked on this quite a few month ago.
I did not publish it as I stumbled upon the already filed issue.
@ofavre
Copy link

ofavre commented Feb 26, 2013

Here is my past work on this issue, before I stumbled upon this pull request.
My memories are there is still a bit work and cleanup to be done.

@ofavre
Copy link

ofavre commented Mar 3, 2013

According to the SQL 2003 standard, we should be able to express sequence of UNION, INTERSECT and EXCEPT. Such sequences should be handled via binary relations for clarity, hence unions (and the rest) should not be chainable, but rather nestable. I'm unsure of what clauses (like LIMIT, ORDER BY) are legal within the so-called [non-join query term](http://savage.net.au/SQL/sql-2003-2.bnf.html#non-join query term), and what clauses are legal in the toplevel union. For instance if ORDER BY is mentioned, LIMIT isn't.

Maybe SelectStatements should nest within unions, or maybe Unions should take the place of SelectCores inside SelectStatements. And, by the way, why do SelectStatement have an array of cores?

What if SelectManager returns a new SelectManager on calls of union? A SelectManager could be either unary (a single SelectStatement) or binary (a Union/Intersect/Except). That may break usual ways of calling it: the instance is no longer modified but wrapped. But that's acceptable. The other solution would be to have SelectUnionManager, SelectIntersectManager and SelectExceptManager.

I think I'll go that direction. Any thoughts? especially on the second paragraph?

@ablignaut
Copy link

ablignaut commented Mar 5, 2013

I wish I was qualified to answer your questions Olivier.
However I don't know enough about the internals of arel yet to comment.

In our project I had to work around this issue by creating a wrapper class around multiple scopes.
The wrapper class takes a base scope in the initializer and then has the methods Intersect, Union and Except which each just store an array of scopes.

I then have an apply method which just builds up the query using string interpolation and calling to_sql on each scope applying all the intersects to the base scope first and then the unions and then the excludes and calling .find_by_sql.

It works for our needs for now. It would however be nice to be able to return an active relation though.

My implementation would lead me to believe that it is a basic implementation of you last paragraph. i.e. a wrapper class around multiple selects.

Sorry I can't help more.

@ofavre ofavre mentioned this pull request Mar 8, 2013
Closed
ofavre added a commit to yakaz/arel that referenced this pull request Mar 17, 2013
@ofavre
Make union/except/intersect chainable as SelectManager
67a6e48 
Fixes rails#118
Fixes rails#98
leonkenneth pushed a commit to leonkenneth/arel that referenced this pull request Mar 21, 2013
@ofavre
Make union/except/intersect chainable as SelectManager
039649e 
Fixes rails#118
Fixes rails#98

Conflicts:
	lib/arel/nodes/and.rb
	lib/arel/nodes/select_statement.rb
	lib/arel/visitors/dot.rb
	lib/arel/visitors/to_sql.rb
	test/nodes/test_or.rb
	test/visitors/test_to_sql.rb
@calebhearth
Copy link

calebhearth commented Apr 13, 2013

Failing test: https://gist.github.com/calebthompson/5377038

jsanders pushed a commit to jsanders/arel that referenced this pull request Jan 15, 2014
@ofavre @jsanders
Make union/except/intersect chainable as SelectManager
130de1f 
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
@jsanders
Copy link

jsanders commented Jan 15, 2014

Updated @ofavre's awesome changeset for 4-0-stable in this branch in case anybody is interested!

@ofavre, @jroes: Do you think it would make sense to pull-request that? It seems to work really well...

@jroes
Copy link
Author

jroes commented Jan 18, 2014

Definitely!

On Wed, Jan 15, 2014 at 1:34 PM, James Sanders notifications@github.comwrote:

Updated @ofavre https://github.com/ofavre's awesome changeset for
4-0-stable in this branchhttps://github.com/jsanders/arel/compare/rails:4-0-stable...fix-118-4-0-stablein case anybody is interested!

@ofavre https://github.com/ofavre, @jroes https://github.com/jroes:
Do you think it would make sense to pull-request that? It seems to work
really well...

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/118#issuecomment-32394426
.

@jsanders jsanders mentioned this pull request Jan 18, 2014
Closed
@ofavre
Copy link

ofavre commented Jan 19, 2014

Due to lack of time, I was actually stuck with an old version of Ruby and could not rebase on up-to-date versions.
I don't remember of any blocker with my branch.
So I 100% back your pull request!

tjgfernandes added a commit to tjgfernandes/arel that referenced this pull request Apr 11, 2014
@tjgfernandes
Chainable unions rails#118 (https://github.com/rails/arel/pull/118/f…
95fb74e 
…iles)
jsanders pushed a commit to jsanders/arel that referenced this pull request Sep 26, 2014
@ofavre @jsanders
Make union/except/intersect chainable as SelectManager
bc8d744 
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
@jsanders jsanders mentioned this pull request Sep 26, 2014
Closed
jsanders pushed a commit to jsanders/arel that referenced this pull request Sep 26, 2014
@ofavre @jsanders
Make union/except/intersect chainable as SelectManager
dc9ecaa 
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/select_manager.rb
jsanders pushed a commit to jsanders/arel that referenced this pull request Sep 4, 2015
@ofavre @jsanders
Make union/except/intersect chainable as SelectManager
0dd9710 
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
@fatkodima fatkodima mentioned this pull request Jan 9, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jroes @ofavre @ablignaut @calebhearth @jsanders