-
Notifications
You must be signed in to change notification settings - Fork 566
Description
Test Failing
NamedScopingTest#test_scopes_honor_current_scopes_from_when_defined
While investigating this failure, I discovered something startling. When windowing over the results, the "expected" primary key order falls away and results are not what you expect. This is normal and proper SQL Server behavior but we should try to add some tie breakers or people are going to freak out. Let me illustrate what this test exposes.
Post.ranked_by_comments.load
SELECT [posts].*
FROM [posts]
ORDER BY comments_count DESC
Take note of the comments_count
order and the happenstantial id
. If we want to take the first 5 rows.
Post.ranked_by_comments.limit_by(5).load
SELECT [posts].*
FROM [posts]
ORDER BY comments_count DESC
OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY
See how rows 3 thru 5 are not the happenstantial/expect ids. For SQL Server, we need to add a tie breaker, like so.
It could be straight forward for us to add a tie breaker order. I experimented with the following in our visitor. The idea would be that we only add this if needed.
def make_Fetch_Possible_And_Deterministic o
return if o.limit.nil? && o.offset.nil?
t = table_From_Statement o
pk = primary_Key_From_Table t
return unless pk
if o.orders.empty?
# Prefer deterministic vs a simple `(SELECT NULL)` expr.
o.orders = [pk.asc]
elsif orders_All_Column_Attributes_And_No_Primary_Key? o, pk
# Force expected Rails deterministic tie breaker if not present.
o.orders << [pk.asc]
end
end
def orders_All_Column_Attributes_And_No_Primary_Key? o, pk
o.orders.all? do |x|
x.respond_to?(:expr) &&
Attributes::Attribute === x.expr &&
begin
!(x.to_sql == pk.asc.to_sql || x.to_sql == pk.desc.to_sql)
rescue ActiveRecord::ConnectionNotEstablished
false
end
end
end
Lastly, to make this EVEN MORE COMPLICATED! I found out that even if we did fix this, Rails will remove the orders for "simple" calculation like so...
def execute_simple_calculation(operation, column_name, distinct) #:nodoc:
# Postgresql doesn't like ORDER BY when there are no GROUP BY
relation = unscope(:order)
Let's do this!