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

ActiveRecord UNION left out #939

Closed
lighthouse-import opened this issue May 16, 2011 · 57 comments
Closed

ActiveRecord UNION left out #939

lighthouse-import opened this issue May 16, 2011 · 57 comments

Comments

@lighthouse-import
Copy link

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/6591
Created by clyfe - 2011-03-17 19:22:47 UTC

ActiveRecord returns Arel::Nodes::Union when I call model.union(active_relation), but it should return an ActiveRelation

  1. What steps will reproduce the problem:

    class User << AR::Base; end
    User.where(:a => 1).union(User.where(:a => 2))

  2. What is the wrong result:

    The result is an instance of Arel::Nodes::Union

  3. What is the result that should happen instead:

    The computation should return an instance of ActiveRelation

THE GOOD

The resulting Arel::Nodes::Union object can be transformed to_sql and then we can use User.find_by_sql(sql)

a = User.where(:a => 1).union(User.where(:a => 2))
sql = a.to_sql # ( SELECT "users".* FROM "users" WHERE "users"."a" = 1 UNION SELECT "users".* FROM "users" WHERE "users"."a" = 2 )"
User.find_by_sql(sql)

THE ISSUE

The issue is obtaining an ActiveRelation object that we can further chain.
Caling a method (select, where, includes) on this (ActiveRelation unioned) object would have the behavior of further calling that method on each of the ActiveRelation objects involved in the UNION


All tough in general a UNION query can be avoided, there are some cases where the corect active relation #union functionality is needed.

For example this wold allow fixing issue #213 of CanCan ryanb/cancan#213

I would be glad to work in this issue, with a little help.

@robmathews
Copy link

Why'd we close this issue? It's a perfectly reasonable request.

@mrcsparker
Copy link

I agree - this is something that would that would be useful.

@ghost
Copy link

ghost commented Nov 5, 2011

I have an app that would benefit from this. Please reopen this issue.

@TheMikeSanto
Copy link

I would also benefit from union returning a relation object. There's been countless times that I need to combine two queries together but still require a relation object. Please reopen.

@alassek
Copy link
Contributor

alassek commented Nov 15, 2011

I had to resort to string interpolation to solve this. A better option would be much appreciated.

@jpemberthy
Copy link

+1 for this one, guess it'd be useful.

@stephenprater
Copy link

+1

@aagrawal2001
Copy link

+1
definitely need this! This is causing a lot of redundant code for me.

@LTe
Copy link
Contributor

LTe commented Dec 16, 2011

+1

3 similar comments
@staszek
Copy link

staszek commented Dec 16, 2011

+1

@bblimke
Copy link

bblimke commented Dec 16, 2011

+1

@paneq
Copy link
Contributor

paneq commented Dec 16, 2011

+1

@arunagw
Copy link
Member

arunagw commented Dec 16, 2011

cc/ @jonleighton

@mlomnicki
Copy link
Contributor

+1

@jonleighton
Copy link
Member

Any of you +1ers are welcome to submit a patch...

@robmathews
Copy link

re patch - Well ... I guess I don't know how. If someone gave me a pointer, I could try. As the original poster said, it needs a little help from somebody who understands the ActiveQuery internals.

@rubypanther
Copy link

Why write a patch if it is CLOSED? Seems like the first step is to flog whoever closed it, the second step is to re-open it, THEN complain that nobody wrote a patch yet.

@rafaelfranca
Copy link
Member

@rubypanther this issue was imported from lighthouse. All the issues imported from lighthouse are closed automatically.

@vsalbaba
Copy link

Please reopen so this will get attention it deserves.

@arunagw arunagw reopened this Apr 26, 2012
@arunagw
Copy link
Member

arunagw commented Apr 26, 2012

It's open. Do it guys!!

@GarPit
Copy link

GarPit commented Jul 9, 2012

+1

@chrisfrommann
Copy link

This would also be incredibly helpful for me. I'd be happy to eventually write a patch if no one else can, but for now, +1...

@fd
Copy link

fd commented Jul 18, 2012

+1

@groe
Copy link

groe commented Aug 2, 2012

I have encountered that problem at some point in 3 projects over the last years. Would be really useful if there was ActiveRecord support for that. +3

ofavre pushed a commit to yakaz/rails that referenced this issue Aug 3, 2012
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 issue Aug 3, 2012
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.
@vkhang55
Copy link

+1

@nunziofiore
Copy link

+1000

@senny
Copy link
Member

senny commented Mar 7, 2013

Is anyone working on this one? Is there a Pull Request?

@ofavre
Copy link

ofavre commented Mar 8, 2013

I proposed some code, but it actually depends on another issue: rails/arel#118.
The right way to solve that issue (and handle other set operations) is still not obvious to me, see this comment, help is welcomed.
I'm planning on proposing a proper pull request for the latter within few weeks, when I find time.
I'll then update my code for this issue and pull request it.
That said, someone can still handle these before I do... ;-)

@ejoubaud
Copy link

ejoubaud commented May 9, 2013

+1

@naveda89
Copy link

naveda89 commented Jun 2, 2013

how is going this issue?? It could be so useful!! :P

@dylanz
Copy link

dylanz commented Jun 24, 2013

+1

1 similar comment
@luke-plewa
Copy link

+1

@russ1985
Copy link

+100

@omgitsfletch
Copy link

+0.9999

@gmccue
Copy link

gmccue commented Sep 24, 2013

would be nice!

@kuraga
Copy link

kuraga commented Oct 5, 2013

Any updates? @ofavre , any updates?

@x3qt
Copy link

x3qt commented Oct 10, 2013

+1

2 similar comments
@mjy
Copy link
Contributor

mjy commented Oct 15, 2013

+1

@mvoidex
Copy link

mvoidex commented Nov 24, 2013

+1

@doshea
Copy link

doshea commented Jan 7, 2014

+1 for this and for #intersect

@ilyakatz
Copy link
Contributor

+1

@rafaelfranca
Copy link
Member

This issue is stale for more than 2 years. Adding +1's will not help and we don't have time to fix all the issues in the world. So I'm closing this one.

Who is having the issue and care about fixing PDI.

@ofavre
Copy link

ofavre commented Jan 19, 2014

Update:
The pull request rails/arel#118, replaced by rails/arel#239 (by @jsanders), exposing the required functionality in Arel is ready to be merged.
Once done, the code at yakaz/rails@29b8ebd (relation-unions-v3.2.2 branch) can be updated and merged.

@rafaelfranca
Copy link
Member

@ofavre thank you for the update. I have assigned the arel PR to me and I'll review it and apply your Rails patch too.

@jsanders
Copy link
Contributor

@rafaelfranca Note that I based the work in rails/arel#239 off of 4-0-stable rather than master. I'm also guessing there is a little work that would need to be done to @ofavre's relation-unions-v3.2.2 branch to make it work against either 4-0-stable or master. Does it make sense to get these changes into 4.0 at this point? If not, I'd be glad to submit PRs against master instead.

@tjgfernandes
Copy link

Have applied @ofavre and @jsanders changes rails/issues/939#issuecomment-32709728 against rails 3-2-stable branch and arel 3-0-stable branch.

These changes are being tested and used with cancancan to combine different ability conditions for same resource by using unions (from @clyfe).

So far changes are working ok for tests and my use cases (rails 3.2.17).

@julianbei
Copy link

+1

@brianhempel
Copy link

I wrote a gem to add usable UNION functionality with ActiveRecord::Relation. We are using it in production. The gem's code may or may not provide a good template for a Rails PR, but it does demonstrate solutions to some of the issues along the way:

  • Don't forget about bind values!
  • Subselects with ORDER BY need to be wrapped in parentheses … except for SQLite, which just explodes in that case
  • The UNION is aliased to the table name to allow further scoping

The important code is here for inspection: https://github.com/brianhempel/active_record_union/blob/master/lib/active_record_union/active_record/relation/union.rb

@guyisra
Copy link

guyisra commented Nov 10, 2014

+1

The problem with using find_by_sql is that it returns an array. However, uglifying it with string interpolation something like this

Model.from("(#{Model.scope1.to_sql}) union (#{Model.scope2.to_sql})) #{Model.table_name}")

will keep it as a relation for further use

@dmitry
Copy link
Contributor

dmitry commented Nov 10, 2014

@brianhempel
Copy link

If you do the union with AREL or SQL string interpolation, watch out for bind values. The SQL for my_user.posts is:

SELECT "posts".* FROM "posts"  WHERE "posts"."user_id" = ?

to_sql won't preserve the value that's supposed to bind to the ?, and I'm pretty sure using AREL won't help you either. So, if you want a useable ActiveRecord::Relation upon which to chain further queries, you may have to do some rebinding.[1]

[1] Like here: https://github.com/brianhempel/active_record_union/blob/master/lib/active_record_union/active_record/relation/union.rb#L25

@asiniy
Copy link

asiniy commented Feb 3, 2015

+1

@rails rails locked and limited conversation to collaborators Feb 3, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests