Skip to content

SELECT DISTINCT clause with ORDER BY for MSSQL + Handling of 'GROUP BY' and selected columns #529

Closed
wants to merge 2 commits into from

3 participants

@jesk
jesk commented Jan 28, 2014

Fixes #437

Jesko added some commits Jan 28, 2014
Jesko Handling of 'GROUP BY' and selected columns
1. Ensure correct queries if the rest_of_query contains a 'GROUP BY'.
2. Create a correct subquery if there's only one id column.
Partially fixes #437
060cde6
Jesko SELECT DISTINCT clause with ORDER BY for MSSQL
SELECT DISTINCT clause for a given set of columns and a given ORDER BY clause.

Partially fixes #437
10637c2
@kares
JRuby Team member
kares commented Jan 28, 2014

Hey Jesko, thank you so much - would it be possible to run the tests (rake test_mssql) if you have not already ?
Most of the time I merge something with MS-SQL esp. if it touches the LimitHelper something else gets broken ... that is also the reason why the tests might be broken currently - so if the same ones fail it's fine just make sure no new ones does ... thanks again!

@jesk
jesk commented Jan 29, 2014

Hi Karol,

thanks for your immediate response. I configured test/db/mssql_config.rb for my MSSQL test database. For both the master and my fork I got several errors and deprecation warnings and the test execution doesn't terminate correctly - maybe a configuration issue.

Running rake test_mssql for the master e.g. shows the following error:

E, [2014-01-29T09:34:12.885000 #3999] ERROR -- : 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, name FROM [long_ships]  GROUP BY name ) AS t WHERE t._row_num BETWEEN 1 AND 2

The good news: This error is fixed with my pull request.

Please check for yourself to confirm this result.

Cheers,
Jesko

@kares
JRuby Team member
kares commented Jan 29, 2014

That's exactly what I was hoping to avoid - to need to run those on my own ;(

How about running the tests in AR < 4.0 (export BUNDLE_GEMFILE/gemfiles/rails32.gemfile) ... last time I checked it sure did not error out, could you try that and report what the output/failures were before and after your changes ? THX

Also I'm really confused by the log in #437 seems that it's @soemo's commits and he seems to have some more in ... any ideas why they show up twice - is there something messy in git ?

@jesk
jesk commented Jan 29, 2014

All right, I will test it again with the other gemfile and inform you.

FYI: I coworked with @soemo and we decided to recommit the changes from my account. My commited changes exactly match the changes he committed. Sorry for the confusion.

@jesk
jesk commented Jan 29, 2014

I tested the fork with several active_record versions. The results can be found here:
https://gist.github.com/jesk/8687668
IMHO it looks promising :)

FYI: The errors which occur when running tests for the fork already exist in the master as well.
The good news are: The fork eliminates one error which exists on the master for all tested ActiveRecord versions (3.0.20, 3.1.12, 3.2.16, 4.0.2, 4.1.0.beta1).

@kares
JRuby Team member
kares commented Jan 29, 2014

excellent! thank you so much for clarifying things and running the tests - expect this in the next release

@kares
JRuby Team member
kares commented Feb 3, 2014

on master now see #437 for commits ... will be in release 1.3.6 ... thank you!

@kares kares closed this Feb 3, 2014
@sanelson2000

This patch appears to have broken something to do with ordering on an aggregate.

I have the following code:

ConsensusWorkspace
        .select('w.*, count(o.object_id) num_objects')
        .joins("w inner join #{ConsensusObject.table_name} o on o.workspace_id = w.workspace_id" )
        .where('w.workspace_id > 0 and w.workspace_id IN (?)', accessible_workspace_ids)
        .group('w.workspace_id, w.workspace, w.description, w.person_id, w.status, w.verification_required, w.approval_process_id, w.team_id, w.last_modified_dt')
        .order('count(o.object_id) DESC')
        .limit(@workspace_count)

In 1.3.5, this generates the following valid query for MSSQL (it also works for Postgres, Oracle, and SQLite):

SELECT t.*
FROM (
    SELECT ROW_NUMBER() OVER (
            ORDER BY count(o.object_id) DESC
            ) AS _row_num
        ,w.*
        ,count(o.object_id) num_objects
    FROM [tp_workspace] w
    INNER JOIN tp_object o ON o.workspace_id = w.workspace_id
    WHERE (
            w.workspace_id > 0
            AND w.workspace_id IN (N'0', N'8', N'9', N'11', N'12', N'13', N'15', N'16', N'18', N'21', N'22', N'23', N'24', N'25', N'26', N'28', N'29', N'30', N'31', N'32', N'33', N'40', N'41', N'42', N'43', N'44', N'45', N'52', N'53', N'54', N'55', N'56', N'57', N'62', N'63', N'64', N'65', N'66', N'67', N'58', N'59', N'60', N'61', N'68', N'69', N'46', N'47', N'48', N'49', N'50', N'51', N'34', N'35', N'36', N'37', N'38', N'39', N'20', N'77', N'78', N'79', N'80', N'81', N'82', N'84', N'86')
            )
    GROUP BY w.workspace_id
        ,w.workspace
        ,w.description
        ,w.person_id
        ,w.STATUS
        ,w.verification_required
        ,w.approval_process_id
        ,w.team_id
        ,w.last_modified_dt
    ) AS t
WHERE t._row_num BETWEEN 1
        AND 10

In 1.3.6 it generates the following invalid SQL:

SELECT t.*
FROM (
    SELECT ROW_NUMBER() OVER (
            ORDER BY MIN(count(o.object_id) DESC)
            ) AS _row_num
        ,w.*
        ,count(o.object_id) num_objects
    FROM [tp_workspace] w
    INNER JOIN tp_object o ON o.workspace_id = w.workspace_id
    WHERE (
            w.workspace_id > 0
            AND w.workspace_id IN (N'0', N'8', N'9', N'11', N'12', N'13', N'15', N'16', N'18', N'21', N'22', N'23', N'24', N'25', N'26', N'28', N'29', N'30', N'31', N'32', N'33', N'40', N'41', N'42', N'43', N'44', N'45', N'52', N'53', N'54', N'55', N'56', N'57', N'62', N'63', N'64', N'65', N'66', N'67', N'58', N'59', N'60', N'61', N'68', N'69', N'46', N'47', N'48', N'49', N'50', N'51', N'34', N'35', N'36', N'37', N'38', N'39', N'20', N'77', N'78', N'79', N'80', N'81', N'82', N'84', N'86')
            )
    GROUP BY w.workspace_id
        ,w.workspace
        ,w.description
        ,w.person_id
        ,w.STATUS
        ,w.verification_required
        ,w.approval_process_id
        ,w.team_id
        ,w.last_modified_dt
    ) AS t
WHERE t._row_num BETWEEN 1
        AND 10

Notice the ORDER BY clause has changed from:

ORDER BY count(o.object_id) DESC

to

ORDER BY MIN(count(o.object_id) DESC)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.