support MySQL and PostgreSQL extended DELETE syntax #228

Open
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants

ccutrer commented Dec 12, 2013

note that this also adds support for limit on all adapters by doing a subquery (just like update)

@ccutrer ccutrer added a commit to ccutrer/rails that referenced this pull request Dec 12, 2013

@ccutrer ccutrer allow a limit for delete_all
also do joins natively for mysql

both require rails/arel#228
a694b65
Owner

jeremy commented Dec 12, 2013

Seeing partial test coverage for the new delete case. There are more behavior changes here, though. Could you expand to full coverage?

ccutrer commented Dec 16, 2013

@jeremy I added additional test coverage.

ccutrer commented Jan 10, 2014

any update on this? do you want the commits squashed together?

tamird commented Sep 16, 2014

@ccutrer needs a rebase

ccutrer commented Sep 16, 2014

@tamird rebased and changed code to match newer code style (using collector in visitors, and compile method in test - which passes a collector)

tamird commented Sep 16, 2014

probably needs a review from @tenderlove

@tenderlove tenderlove and 1 other commented on an outdated diff Oct 10, 2014

lib/arel/nodes/delete_statement.rb
def initialize relation = nil, wheres = []
- super
@tenderlove

tenderlove Oct 10, 2014

Owner

Did you mean to remove the call to super?

@ccutrer

ccutrer Oct 10, 2014

I did not. probably got confused in the conflicts when I rebased

@tenderlove tenderlove and 1 other commented on an outdated diff Oct 10, 2014

lib/arel/visitors/postgresql.rb
@@ -23,6 +23,45 @@ def visit_Arel_Nodes_DistinctOn o, collector
collector << "DISTINCT ON ( "
visit(o.expr, collector) << " )"
end
+
+ def visit_Arel_Nodes_DeleteStatement o, collector
+ return super if !o.relation.is_a?(Arel::Nodes::JoinSource) || o.relation.right.empty?
@tenderlove

tenderlove Oct 10, 2014

Owner

Why are we doing is_a? checks here?

@ccutrer

ccutrer Oct 10, 2014

it's checking if there's a join. if there is, it does the special syntax here. otherwise, it just calls super and let's the default implementation have it. do you prefer something besides is_a? ?

tamird commented Feb 24, 2015

@ccutrer #356 was just merged, can you rebase?

tamird commented Feb 26, 2015

@ccutrer can you squash the relevant parts of the last commit into the other two? the changes look good to me, @rafaelfranca can you please triage?

ccutrer added some commits Dec 12, 2013

@ccutrer ccutrer support Mysql extended DELETE syntax
this also fixes the general case by using a subselect rather than a
direct limit (which is not standard SQL)
494b701
@ccutrer @ccutrer ccutrer support for postgresql multi-table update and delete syntax 18be55a

ccutrer commented Feb 26, 2015

@tamird - squashed down to two commits.

tamird commented Feb 26, 2015

would love to see this get in 👍 cc @tenderlove

Owner

tenderlove commented Mar 2, 2015

LGTM, but @sgrif can you review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment