Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix pagination on SQL Server 2000 with GROUP clause (Fix #367) #369

Closed
wants to merge 2 commits into from

2 participants

@pschambacher

The pagination condition was added before the ORDER condition.
If there is a GROUP BY clause, the pagination was in the GROUP BY
instead of the WHERE clause. New approach is extracting each clause and
build a new proper query.

@pschambacher pschambacher Fix #367 pagination not working with group on SQLSERVER 2000.
The pagination condition was added before the ORDER condition.
If there is a GROUP BY clause, the pagination was in the GROUP BY
instead of the WHERE clause. New approach is extraction each clause and
build a new proper query.
4bed7eb
@kares
Collaborator

fix is on master as 8e85d2f but the newly added test fails for me on 2012 :

Error: test_limit_with_group_by(MSSQLLimitOffsetTest)
  ActiveRecord::StatementInvalid: ActiveRecord::JDBCError: Column 'long_ships.id' is invalid in the select list because it is not contained in either an aggregate function or the GROUP BY clause.: SELECT t.* FROM (SELECT ROW_NUMBER() OVER(ORDER BY long_ships.id) AS _row_num, [long_ships].* FROM [long_ships]  GROUP BY name) AS t WHERE t._row_num BETWEEN 1 AND 2
arjdbc/jdbc/RubyJdbcConnection.java:439:in `execute_query_raw'
/home/kares/workspace/github/activerecord-jdbc-adapter/lib/arjdbc/jdbc/adapter.rb:283:in `exec_query_raw'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activerecord-3.2.13/lib/active_record/connection_adapters/abstract_adapter.rb:280:in `log'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activesupport-3.2.13/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activesupport-3.2.13/lib/active_support/notifications/instrumenter.rb:19:in `instrument'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activerecord-3.2.13/lib/active_record/connection_adapters/abstract_adapter.rb:275:in `log'
/home/kares/workspace/github/activerecord-jdbc-adapter/lib/arjdbc/jdbc/adapter.rb:282:in `exec_query_raw'
/home/kares/workspace/github/activerecord-jdbc-adapter/lib/arjdbc/jdbc/adapter.rb:301:in `select'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activerecord-3.2.13/lib/active_record/connection_adapters/abstract/database_statements.rb:18:in `select_all'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activerecord-3.2.13/lib/active_record/connection_adapters/abstract/query_cache.rb:63:in `select_all'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activerecord-3.2.13/lib/active_record/querying.rb:38:in `find_by_sql'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activerecord-3.2.13/lib/active_record/explain.rb:41:in `logging_query_plan'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activerecord-3.2.13/lib/active_record/querying.rb:37:in `find_by_sql'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activerecord-3.2.13/lib/active_record/relation.rb:171:in `exec_queries'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activerecord-3.2.13/lib/active_record/relation.rb:160:in `to_a'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activerecord-3.2.13/lib/active_record/explain.rb:41:in `logging_query_plan'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activerecord-3.2.13/lib/active_record/relation.rb:159:in `to_a'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activerecord-3.2.13/lib/active_record/relation/finder_methods.rb:159:in `all'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activerecord-3.2.13/lib/active_record/relation/finder_methods.rb:105:in `find'
/opt/local/rvm/gems/jruby-1.7.3@jdbc/gems/activerecord-3.2.13/lib/active_record/relation/finder_methods.rb:101:in `find'
/home/kares/workspace/github/activerecord-jdbc-adapter/test/db/mssql/limit_offset_test.rb:177:in `test_limit_with_group_by'
     174:       LongShip.create!(:name => name)
     175:     end
     176: 
  => 177:     ships = LongShip.group(:name).find(:all, :limit => 2)

could you please look at it if the failure occurs at your side as well ?
might be caused by refactorings done since you updated master ...

@pschambacher

Oh yeah I forgot once again that SQL Server doesn't know how to deal with this ... I'll fix this !

@pschambacher

Sorry about that.

Is there a documentation somewhere how to configure your computer and database to run the tests ?
This way I would be able to run them locally (at least for MSSQL2000 and 2005)

@kares
Collaborator

not really but if you go through the tests you'll find out, this should be sufficient to read to get started :
https://github.com/jruby/activerecord-jdbc-adapter/blob/master/test/db/mssql.rb

@kares kares closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 9, 2013
  1. @pschambacher

    Fix #367 pagination not working with group on SQLSERVER 2000.

    pschambacher authored
    The pagination condition was added before the ORDER condition.
    If there is a GROUP BY clause, the pagination was in the GROUP BY
    instead of the WHERE clause. New approach is extraction each clause and
    build a new proper query.
  2. @pschambacher

    Fix test for SQL Server.

    pschambacher authored
This page is out of date. Refresh to see the latest.
View
2  lib/arjdbc/mssql/adapter.rb
@@ -685,4 +685,4 @@ module ActiveRecord::ConnectionAdapters
class MSSQLColumn < JdbcColumn
include ArJdbc::MSSQL::Column
end
-end
+end
View
43 lib/arjdbc/mssql/limit_helpers.rb
@@ -68,22 +68,49 @@ def replace_limit_offset!(sql, limit, offset, order)
new_sql = "#{select} TOP 1 #{rest_of_query} #{new_order}"
sql.replace(new_sql)
else
- #UGLY
- #KLUDGY?
- #removing out stuff before the FROM...
- rest = rest_of_query[/FROM/i=~ rest_of_query.. -1]
+ # We are in deep trouble here. SQL Server does not have any kind of OFFSET build in.
+ # Only remaining solution is adding a where condition to be sure that the ID is not in SELECT TOP OFFSET FROM SAME_QUERY.
+ # To do so we need to extract each part of the query to insert our additional condition in the right place.
+ query_without_select = rest_of_query[/FROM/i=~ rest_of_query.. -1]
+ additional_condition = "#{table_name}.#{primary_key} NOT IN (#{select} TOP #{offset} #{table_name}.#{primary_key} #{query_without_select} #{new_order})"
- if (rest_of_query.match(/WHERE/).nil?)
- new_sql = "#{select} TOP #{limit} #{rest_of_query} WHERE #{table_name}.#{primary_key} NOT IN (#{select} TOP #{offset} #{table_name}.#{primary_key} #{rest} #{new_order}) #{order} "
+ # Extract the different parts of the query
+ having, group_by, where, from, selection = split_sql(rest_of_query, /having/i, /group by/i, /where/i, /from/i)
+
+ # Update the where part to add our additional condition
+ if where.blank?
+ where = "WHERE #{additional_condition}"
else
- new_sql = "#{select} TOP #{limit} #{rest_of_query} AND #{table_name}.#{primary_key} NOT IN (#{select} TOP #{offset} #{table_name}.#{primary_key} #{rest} #{new_order}) #{order} "
+ where = "#{where} AND #{additional_condition}"
end
- sql.replace(new_sql)
+ # Replace the query to be our new customized query
+ sql.replace("#{select} TOP #{limit} #{selection} #{from} #{where} #{group_by} #{having} #{new_order}")
end
end
sql
end
+
+ # Split the rest_of_query into chunks based on regexs (applied from end of string to the beginning)
+ # The result is an array of regexs.size+1 elements (the last one being the remaining once everything was chopped away)
+ def split_sql rest_of_query, *regexs
+ results = Array.new
+
+ regexs.each do |regex|
+ if position = (regex =~ rest_of_query)
+ # Extract the matched string and chop the rest_of_query
+ matched = rest_of_query[position..-1]
+ rest_of_query = rest_of_query[0...position]
+ else
+ matched = nil
+ end
+
+ results << matched
+ end
+ results << rest_of_query
+
+ results
+ end
def get_primary_key(order, table_name) # table_name might be quoted
if order =~ /(\w*id\w*)/i
View
8 test/db/mssql/limit_offset_test.rb
@@ -169,4 +169,12 @@ def test_offset_without_limit
assert_equal("You must specify :limit with :offset.", error.message)
end
+ def test_limit_with_group_by
+ %w(one two three four five six seven eight).each do |name|
+ LongShip.create!(:name => name)
+ end
+
+ ships = LongShip.group(:name).select(:name).find(:all, :limit => 2)
+ asset_equal(['one', 'two'], ships.map(&:name))
+ end
end
Something went wrong with that request. Please try again.