Skip to content

Conversation

@aidanharan
Copy link
Contributor

@aidanharan aidanharan commented Apr 22, 2021

This PR fixes the following failing CI tests:

  • CalculationsTest#test_select_avg_with_group_by_as_virtual_attribute_with_ar
  • CalculationsTest#test_select_avg_with_joins_and_group_by_as_virtual_attribute_with_ar
  • CalculationsTest#test_select_avg_with_group_by_as_virtual_attribute_with_sql
  • CalculationsTest#test_select_avg_with_joins_and_group_by_as_virtual_attribute_with_sql

The tests needed to be coerced and re-implemented for the following reasons:

AVG() function
In SQL Server the AVG() function for a list of integers returns an integer. For other databases it would return a decimal. To get SQL Server to return a decimal result the values need to be casted before averaging. so need to cast values as decimals before averaging. So instead of AVG(credit_limit) AS avg_credit_limit we need to use AVG(CAST(credit_limit AS DECIMAL) AS avg_credit_limit.

SELECT columns must be in the GROUP clause too
SQL Servers queries cannot be:

SELECT companies.*
...
GROUP BY companies.id

They need to include all the company's columns in the GROUP clause like:

SELECT companies.*
...
GROUP BY companies.id, companies.type, companies.firm_id, companies.firm_name, companies.name, companies.client_of, companies.rating, companies.account_id, companies.description

LIMIT clause
SQL Server does not use the LIMIT clause but FETCH instead.

@aidanharan aidanharan marked this pull request as ready for review April 22, 2021 13:39
@aidanharan aidanharan changed the title Coerce some calculation tests Rails 6.1: Fix failing calculation tests Apr 22, 2021
@aidanharan aidanharan marked this pull request as draft April 22, 2021 13:50
@aidanharan aidanharan marked this pull request as ready for review April 22, 2021 14:01
@aidanharan aidanharan marked this pull request as draft April 22, 2021 14:01
@aidanharan aidanharan force-pushed the calculation-tests-coerced branch from 22b74a1 to c7a30dc Compare April 22, 2021 14:03
@aidanharan aidanharan marked this pull request as ready for review April 22, 2021 14:12
@wpolicarpo wpolicarpo merged commit 0e0da50 into rails-sqlserver:main Apr 22, 2021
lavika pushed a commit to lavika/activerecord-sqlserver-adapter that referenced this pull request Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants