pg_connection.distinct refactoring #7562

Closed
wants to merge 11 commits into
from

Projects

None yet

4 participants

@semaperepelitsa
Contributor

I have covered pg_connection.distinct with tests and refactored it. I would also like to rebase this against 3-2-stable because master branch contains fix for #5868, I just cleaned up and made sure it works.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Sep 7, 2012
...d/connection_adapters/postgresql/schema_statements.rb
- # Construct a clean list of column names from the ORDER BY clause, removing
- # any ASC/DESC modifiers
- order_columns = orders.collect do |s|
- s = s.to_sql unless s.is_a?(String)
- s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '')
- end
- order_columns.delete_if { |c| c.blank? }
- order_columns = order_columns.zip((0...order_columns.size).to_a).map { |s,i| "#{s} AS alias_#{i}" }
-
- "DISTINCT #{columns}, #{order_columns * ', '}"
+ order_columns = orders.
+ # Convert Arel nodes to strings
+ map{ |node| node.respond_to?(:to_sql) ? node.to_sql : node }.
+ # Construct a clean list of column names from the ORDER BY clause, removing
+ # any ASC/DESC modifiers
+ map{ |s| s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '') }.
carlosantoniodasilva
carlosantoniodasilva Sep 7, 2012 Owner

I don't think we need to iterate the orders twice here, probably one iteration would solve it?

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Sep 7, 2012
...d/connection_adapters/postgresql/schema_statements.rb
- # any ASC/DESC modifiers
- order_columns = orders.collect do |s|
- s = s.to_sql unless s.is_a?(String)
- s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '')
- end
- order_columns.delete_if { |c| c.blank? }
- order_columns = order_columns.zip((0...order_columns.size).to_a).map { |s,i| "#{s} AS alias_#{i}" }
-
- "DISTINCT #{columns}, #{order_columns * ', '}"
+ order_columns = orders.
+ # Convert Arel nodes to strings
+ map{ |node| node.respond_to?(:to_sql) ? node.to_sql : node }.
+ # Construct a clean list of column names from the ORDER BY clause, removing
+ # any ASC/DESC modifiers
+ map{ |s| s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '') }.
+ reject(&:blank?).
carlosantoniodasilva
carlosantoniodasilva Sep 7, 2012 Owner

I believe delete_if is fine, or reject!, since it does not create another object.

semaperepelitsa
semaperepelitsa Sep 7, 2012 Contributor

This chained and one-thing-per-enumaration code looks much better for me. Since there are usually no more than few order clauses, the performance difference should be negligible.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Sep 7, 2012
...d/connection_adapters/postgresql/schema_statements.rb
- order_columns = orders.collect do |s|
- s = s.to_sql unless s.is_a?(String)
- s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '')
- end
- order_columns.delete_if { |c| c.blank? }
- order_columns = order_columns.zip((0...order_columns.size).to_a).map { |s,i| "#{s} AS alias_#{i}" }
-
- "DISTINCT #{columns}, #{order_columns * ', '}"
+ order_columns = orders.
+ # Convert Arel nodes to strings
+ map{ |node| node.respond_to?(:to_sql) ? node.to_sql : node }.
+ # Construct a clean list of column names from the ORDER BY clause, removing
+ # any ASC/DESC modifiers
+ map{ |s| s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '') }.
+ reject(&:blank?).
+ each_with_index.map { |column, i| "#{column} AS alias_#{i}" }
carlosantoniodasilva
carlosantoniodasilva Sep 7, 2012 Owner

Here you could use map.with_index instead.

Thanks. I've made a few comments, let me know if you need anything.

The fix you talk about, you mean it's already fixed in master, or you're trying to fix it?

Contributor

Thanks for reviewing. The fix is already in master but I didn't see any tests for it. Now I'm sure it works.

Contributor

The test is called test_distinct_with_arel_order

@tenderlove tenderlove and 1 other commented on an outdated diff Sep 7, 2012
...d/connection_adapters/postgresql/schema_statements.rb
def distinct(columns, orders) #:nodoc:
- return "DISTINCT #{columns}" if orders.empty?
-
- # Construct a clean list of column names from the ORDER BY clause, removing
- # any ASC/DESC modifiers
- order_columns = orders.collect do |s|
- s = s.to_sql unless s.is_a?(String)
- s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '')
- end
- order_columns.delete_if { |c| c.blank? }
- order_columns = order_columns.zip((0...order_columns.size).to_a).map { |s,i| "#{s} AS alias_#{i}" }
-
- "DISTINCT #{columns}, #{order_columns * ', '}"
+ order_columns = orders.
+ # Convert Arel nodes to strings
+ map{ |node| node.respond_to?(:to_sql) ? node.to_sql : node }.
tenderlove
tenderlove Sep 7, 2012 Owner

I'd rather we don't break up lines by dots. It's annoying to figure out who the recipient of the map like this.

Why are we checking respond_to? on to_sql here? The previous code did not do that.

semaperepelitsa
semaperepelitsa Sep 7, 2012 Contributor

I would place a dot in the beginning of each line but Ruby parser can't handle comments in-between :-( The receiver is the previous line, I've made an indentation to help people see that.

The previous code converted to_sql everything but strings: s = s.to_sql unless s.is_a?(String). I'm doing essentially the same thing, except I ask the object if it can convert itself rather than check type.

tenderlove
tenderlove Sep 7, 2012 Owner

I would place a dot in the beginning of each line but Ruby parser can't handle comments in-between :-( The receiver is the previous line, I've made an indentation to help people see that.

Put the comment inside the block. e.g.:

order_columns = orders.map { |node|
  # Convert Arel nodes to strings
  node.respond_to?(:to_sql) ? node.to_sql : node
}

The call to map isn't actually doing the conversion, it's building an array. The call to to_sql is what your comment is actually about.

The previous code converted to_sql everything but strings: s = s.to_sql unless s.is_a?(String). I'm doing essentially the same thing, except I ask the object if it can convert itself rather than check type.

It's still different behavior. Removing the check for String means that we are increasing the number of types of objects that we accept (is it now OK to add Arrays and Symbols, etc?). Unless you're actually fixing a bug, please don't change the logic.

Owner

@semaperepelitsa ping. Could you address @tenderlove concerns and we move this ahead?

semaperepelitsa added some commits Nov 21, 2012
@semaperepelitsa semaperepelitsa Don't break up lines by dots.
Move comments closer to the code performing the actual action.
fd06c80
@semaperepelitsa semaperepelitsa Revert calling to_sql only if responds since this changes existing be…
…haviour and does not fix anything.
9ef9c93
Contributor

Done. Does this look good now?

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Nov 21, 2012
...d/connection_adapters/postgresql/schema_statements.rb
- # Construct a clean list of column names from the ORDER BY clause, removing
- # any ASC/DESC modifiers
- order_columns = orders.collect do |s|
- s = s.to_sql unless s.is_a?(String)
- s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '')
- end
- order_columns.delete_if { |c| c.blank? }
- order_columns = order_columns.zip((0...order_columns.size).to_a).map { |s,i| "#{s} AS alias_#{i}" }
-
- "DISTINCT #{columns}, #{order_columns * ', '}"
+ order_columns = orders.map{ |node|
+ # Convert Arel node to string
+ node.is_a?(String) ? node : node.to_sql
+ }.map{ |s|
+ # Remove any ASC/DESC modifiers
+ s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '')
rafaelfranca
rafaelfranca Nov 21, 2012 Owner

I'd only do one iteration to convert the Arel node and Remove the ASC/DESC modifiers. WDYT?

semaperepelitsa
semaperepelitsa Nov 21, 2012 Contributor

I thought it might be more clear to do one thing at a time, but this doesn't really matter.

Owner

Could you squash your commits?

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