Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

1.3.6 Breaks ordering on an aggregate in MSSQL #532

Closed
sanelson2000 opened this issue Feb 13, 2014 · 4 comments · Fixed by #549
Closed

1.3.6 Breaks ordering on an aggregate in MSSQL #532

sanelson2000 opened this issue Feb 13, 2014 · 4 comments · Fixed by #549
Assignees
@kares
Labels
mssql
Milestone
1.3.18

Comments

@sanelson2000
Copy link

I previously added this as a comment to #529, but I think maybe I should have just created a new bug for it, so that's what I'm doing.

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)
@kares kares added the mssql label Feb 16, 2014
@kares
Copy link
Member

kares commented Feb 26, 2014

... maybe @jesk and/or @soemo have something to say/do about this one?

iaddict added a commit to iaddict/activerecord-jdbc-adapter that referenced this issue Mar 25, 2014
@iaddict
Fix ordering on an aggregate in MSSQL
ad5c3dc 
Ensure that an order clause which contains aggregate functions does not get manipulated.
This should fix jruby#532
@iaddict iaddict mentioned this issue Mar 25, 2014
Merged
@kares kares closed this as completed in #549 Apr 3, 2014
@sanelson2000
Copy link
Author

Sorry, I just now got around to updating to 1.3.7 to test this, but this issue is still present. I had to revert back to 1.3.5 again.

@kares kares reopened this Jun 13, 2014
@fpauser
Copy link
Contributor

fpauser commented Jul 30, 2014

Same problem with 1.3.9. After downgrading to 1.3.5 it works.

@kares kares closed this as completed in b018da4 Jul 30, 2014
This was referenced Apr 29, 2015
Closed
Closed
@kares kares reopened this May 6, 2015
@kares kares self-assigned this Sep 11, 2015
@kares kares added this to the 1.3.18 milestone Sep 11, 2015
kares added a commit to kares/activerecord-jdbc-adapter that referenced this issue Sep 11, 2015
@kares
[mssql] fix broken ordering on an aggregate queries (test for fix jru…
4c5522a 
…by#646 closing jruby#532)
@kares
Copy link
Member

kares commented Sep 12, 2015

merged #646 as 75829cf + a646f29 and a test: 4c5522a (on 1-3-stable)

@kares kares closed this as completed Sep 12, 2015
kares added a commit that referenced this issue Sep 14, 2015
@kares
Merge branch '1-3-stable'
4190f60 
* 1-3-stable: (152 commits)
  1.3.18 is happening :now:
  warn on mimer and informix adapters as unsupported on 4.2
  re-built adapter jar
  preparing for 1.3.18
  fill in change-log
  as PG's BC test refactoring revealed insert_returning config is failing - pend
  since arel visitor instances might get re-used we need to avoid the @instance variable
  [mssql] there are 3 hard problems in it: naming, caching and SQLServer AREL visitors !
  [mssql] re-arrange visitor to only fetch PK if really needed
  handle regression - it's :datetime won't exists on PG :timestamp will ...
  [postgres] work-around BC time parsing issue (on JRuby 1.7.x) by a patch
  make sure Postgre's native part does type-cast on timestamp2ruby (for correctness)
  [mssql] fix broken ordering on an aggregate queries (test for fix #646 closing #532)
  tune replace limit/offset code (merged from #646) to produce less temporary objects
  Fix broken limit_helpers
  deprecate attr writers on column instance
  handle id updatting with partial updates disabled test on AR >= 4.0
  remove FIXME note - type_cast_from_database already goes along + tune doc
  [mssql] extract default values such as ((-1)) returned from JDBC
  change typos in test names
  ...

Conflicts:
	.travis.yml
	lib/arjdbc/firebird/adapter.rb
	lib/arjdbc/jdbc/adapter_java.jar
	lib/arjdbc/mimer.rb
	lib/arjdbc/mssql/adapter.rb
	lib/arjdbc/oracle/adapter.rb
	lib/arjdbc/version.rb
	rakelib/02-test.rake
	src/java/arjdbc/jdbc/RubyJdbcConnection.java
	src/java/arjdbc/mssql/MSSQLRubyJdbcConnection.java
	src/java/arjdbc/oracle/OracleRubyJdbcConnection.java
	src/java/arjdbc/postgresql/PostgreSQLRubyJdbcConnection.java
	test/db/firebird/simple_test.rb
	test/jndi_callbacks_test.rb
	test/simple.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kares kares

Labels
mssql
Projects
None yet
Milestone
1.3.18
Development

Successfully merging a pull request may close this issue.

Fix ordering on an aggregate in MSSQL iaddict/activerecord-jdbc-adapter
3 participants
@fpauser @kares @sanelson2000