From 847752a295d411ccff31f3137c140ec0f5445c07 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Mon, 20 May 2013 18:02:46 -0700 Subject: [PATCH 1/5] partition the where values so we can access the removed ones --- activerecord/lib/active_record/relation/merger.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb index 98afeb8cc5..58ac239190 100644 --- a/activerecord/lib/active_record/relation/merger.rb +++ b/activerecord/lib/active_record/relation/merger.rb @@ -145,12 +145,17 @@ def merged_wheres # Remove equalities from the existing relation with a LHS which is # present in the relation being merged in. def reject_overwrites(lhs_wheres, rhs_wheres) + partition_overwrites(lhs_wheres, rhs_wheres).last + end + + def partition_overwrites(lhs_wheres, rhs_wheres) nodes = rhs_wheres.find_all do |w| w.respond_to?(:operator) && w.operator == :== end seen = Set.new(nodes) { |node| node.left } - lhs_wheres.reject do |w| + # returns [deleted, keepers] + lhs_wheres.partition do |w| w.respond_to?(:operator) && w.operator == :== && seen.include?(w.left) end end From a483ae6477fab482e95b3415bb9b0cf54f198699 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 21 May 2013 10:35:35 -0700 Subject: [PATCH 2/5] push partion logic down and initialization logic up --- .../lib/active_record/relation/merger.rb | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb index 58ac239190..eb72d551da 100644 --- a/activerecord/lib/active_record/relation/merger.rb +++ b/activerecord/lib/active_record/relation/merger.rb @@ -99,7 +99,10 @@ def merge_joins end def merge_multi_values - relation.where_values = merged_wheres + rhs_wheres = values[:where] || [] + lhs_wheres = relation.where_values + + relation.where_values = merged_wheres(lhs_wheres, rhs_wheres) relation.bind_values = merged_binds if values[:reordering] @@ -131,30 +134,23 @@ def merged_binds end end - def merged_wheres - rhs_wheres = values[:where] || [] - lhs_wheres = relation.where_values - - if rhs_wheres.empty? || lhs_wheres.empty? - lhs_wheres + rhs_wheres - else - reject_overwrites(lhs_wheres, rhs_wheres) + rhs_wheres - end + def merged_wheres(lhs_wheres, rhs_wheres) + partition_overwrites(lhs_wheres, rhs_wheres).last + rhs_wheres end # Remove equalities from the existing relation with a LHS which is # present in the relation being merged in. - def reject_overwrites(lhs_wheres, rhs_wheres) - partition_overwrites(lhs_wheres, rhs_wheres).last - end - + # returns [things_to_remove, things_to_keep] def partition_overwrites(lhs_wheres, rhs_wheres) + if lhs_wheres.empty? || rhs_wheres.empty? + return [[], lhs_wheres] + end + nodes = rhs_wheres.find_all do |w| w.respond_to?(:operator) && w.operator == :== end seen = Set.new(nodes) { |node| node.left } - # returns [deleted, keepers] lhs_wheres.partition do |w| w.respond_to?(:operator) && w.operator == :== && seen.include?(w.left) end From d2d5f15f0729d00132a240dd9e5169dce5962a0d Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 21 May 2013 10:38:51 -0700 Subject: [PATCH 3/5] push partitioning up so bind elimination can get the removed wheres --- activerecord/lib/active_record/relation/merger.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb index eb72d551da..38f49912d1 100644 --- a/activerecord/lib/active_record/relation/merger.rb +++ b/activerecord/lib/active_record/relation/merger.rb @@ -102,7 +102,9 @@ def merge_multi_values rhs_wheres = values[:where] || [] lhs_wheres = relation.where_values - relation.where_values = merged_wheres(lhs_wheres, rhs_wheres) + _, kept = partition_overwrites(lhs_wheres, rhs_wheres) + + relation.where_values = kept + rhs_wheres relation.bind_values = merged_binds if values[:reordering] @@ -134,10 +136,6 @@ def merged_binds end end - def merged_wheres(lhs_wheres, rhs_wheres) - partition_overwrites(lhs_wheres, rhs_wheres).last + rhs_wheres - end - # Remove equalities from the existing relation with a LHS which is # present in the relation being merged in. # returns [things_to_remove, things_to_keep] From 50823452f70341447a010df27dd514fd97bfe8a9 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 21 May 2013 10:55:56 -0700 Subject: [PATCH 4/5] remove bind values for where clauses that were removed --- .../lib/active_record/relation/merger.rb | 17 ++++++++--------- activerecord/test/cases/relations_test.rb | 10 ++++++++++ 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb index 38f49912d1..a65ef1898a 100644 --- a/activerecord/lib/active_record/relation/merger.rb +++ b/activerecord/lib/active_record/relation/merger.rb @@ -99,13 +99,15 @@ def merge_joins end def merge_multi_values - rhs_wheres = values[:where] || [] lhs_wheres = relation.where_values + rhs_wheres = values[:where] || [] + lhs_binds = relation.bind_values + rhs_binds = values[:bind] || [] - _, kept = partition_overwrites(lhs_wheres, rhs_wheres) + removed, kept = partition_overwrites(lhs_wheres, rhs_wheres) relation.where_values = kept + rhs_wheres - relation.bind_values = merged_binds + relation.bind_values = filter_binds(lhs_binds, removed) + rhs_binds if values[:reordering] # override any order specified in the original relation @@ -128,12 +130,9 @@ def merge_single_values end end - def merged_binds - if values[:bind] - (relation.bind_values + values[:bind]).uniq(&:first) - else - relation.bind_values - end + def filter_binds(lhs_binds, removed_wheres) + set = Set.new removed_wheres.map { |x| x.left.name } + lhs_binds.dup.delete_if { |col,_| set.include? col.name } end # Remove equalities from the existing relation with a LHS which is diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index cf6af4e8f4..3c1fae78ce 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -1546,4 +1546,14 @@ def __omg__ assert merged.to_sql.include?("wtf") assert merged.to_sql.include?("bbq") end + + def test_merging_removes_rhs_bind_parameters + left = Post.where(id: Arel::Nodes::BindParam.new('?')) + column = Post.columns_hash['id'] + left.bind_values += [[column, 20]] + right = Post.where(id: 10) + + merged = left.merge(right) + assert_equal [], merged.bind_values + end end From f3ebbeae6eb647767ccd49e25821b1ba33923596 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 21 May 2013 11:01:19 -0700 Subject: [PATCH 5/5] avoid creating a set if no where values are removed --- activerecord/lib/active_record/relation/merger.rb | 2 ++ activerecord/test/cases/relations_test.rb | 12 ++++++++++++ 2 files changed, 14 insertions(+) diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb index a65ef1898a..c114ea0c0d 100644 --- a/activerecord/lib/active_record/relation/merger.rb +++ b/activerecord/lib/active_record/relation/merger.rb @@ -131,6 +131,8 @@ def merge_single_values end def filter_binds(lhs_binds, removed_wheres) + return lhs_binds if removed_wheres.empty? + set = Set.new removed_wheres.map { |x| x.left.name } lhs_binds.dup.delete_if { |col,_| set.include? col.name } end diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 3c1fae78ce..b64ff13d29 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -1556,4 +1556,16 @@ def test_merging_removes_rhs_bind_parameters merged = left.merge(right) assert_equal [], merged.bind_values end + + def test_merging_keeps_lhs_bind_parameters + column = Post.columns_hash['id'] + binds = [[column, 20]] + + right = Post.where(id: Arel::Nodes::BindParam.new('?')) + right.bind_values += binds + left = Post.where(id: 10) + + merged = left.merge(right) + assert_equal binds, merged.bind_values + end end