Skip to content
Browse files

Merge branch 'bindwhere'

* bindwhere:
  avoid creating a set if no where values are removed
  remove bind values for where clauses that were removed
  push partitioning up so bind elimination can get the removed wheres
  push partion logic down and initialization logic up
  partition the where values so we can access the removed ones
  • Loading branch information...
2 parents f47b423 + f3ebbea commit ac70ec64138765ddd2a7c5e0130243e6df98cf2d @tenderlove tenderlove committed
Showing with 42 additions and 20 deletions.
  1. +20 −20 activerecord/lib/active_record/relation/merger.rb
  2. +22 −0 activerecord/test/cases/relations_test.rb
View
40 activerecord/lib/active_record/relation/merger.rb
@@ -99,8 +99,15 @@ def merge_joins
end
def merge_multi_values
- relation.where_values = merged_wheres
- relation.bind_values = merged_binds
+ lhs_wheres = relation.where_values
+ rhs_wheres = values[:where] || []
+ lhs_binds = relation.bind_values
+ rhs_binds = values[:bind] || []
+
+ removed, kept = partition_overwrites(lhs_wheres, rhs_wheres)
+
+ relation.where_values = kept + rhs_wheres
+ relation.bind_values = filter_binds(lhs_binds, removed) + rhs_binds
if values[:reordering]
# override any order specified in the original relation
@@ -123,34 +130,27 @@ def merge_single_values
end
end
- def merged_binds
- if values[:bind]
- (relation.bind_values + values[:bind]).uniq(&:first)
- else
- relation.bind_values
- end
- end
-
- def merged_wheres
- rhs_wheres = values[:where] || []
- lhs_wheres = relation.where_values
+ def filter_binds(lhs_binds, removed_wheres)
+ return lhs_binds if removed_wheres.empty?
- if rhs_wheres.empty? || lhs_wheres.empty?
- lhs_wheres + rhs_wheres
- else
- reject_overwrites(lhs_wheres, rhs_wheres) + rhs_wheres
- end
+ 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
# present in the relation being merged in.
- def reject_overwrites(lhs_wheres, rhs_wheres)
+ # 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 }
- lhs_wheres.reject do |w|
+ lhs_wheres.partition do |w|
w.respond_to?(:operator) && w.operator == :== && seen.include?(w.left)
end
end
View
22 activerecord/test/cases/relations_test.rb
@@ -1546,4 +1546,26 @@ 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
+
+ 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

0 comments on commit ac70ec6

Please sign in to comment.
Something went wrong with that request. Please try again.