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.

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

Query: OrderBy constant generates incorrect query for integer/boolean #6145

Closed
smitpatel opened this issue Jul 23, 2016 · 9 comments
Closed
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@smitpatel
Copy link
Contributor

smitpatel commented Jul 23, 2016

At present in query,
OrderBy(c => true) translates to ORDER BY 1
OrderBy(c => 3) translates to ORDER BY 3
In SqlServer, ORDER BY integer means order by that column number from projection which introduces unintended ordering and gives different results compared to what linq gives.
We are also using creating Order by when we do group join with default if empty using the left side of join condition which also faces above issue based on the join condition.
See tests QuerySqlServerTest.OrderBy_true, QuerySqlServerTest.OrderBy_integer & GearsOfWarQuerySqlServerTest.Left_join_predicate_value_equals_condition
Possible work-arounds:
We can remove the order by clause if it is using constant. Results would match this way with linq but for paging cases, order by clause is necessary.
We can introduce dummy order by as follows:
Order by @@ROWCOUNT
Order by @parameter + 1 where @parameter is passed. Value doesn't matter.
@divega

@smitpatel smitpatel changed the title OrderBy true generates incorrect query OrderBy constant generates incorrect query for integer/boolean Jul 23, 2016
@divega divega changed the title OrderBy constant generates incorrect query for integer/boolean SQL Server: OrderBy constant generates incorrect query for integer/boolean Jul 23, 2016
@divega
Copy link
Contributor

divega commented Jul 23, 2016

I think the cases of ORDER BY with a non-numeric and non-Boolean constants and ORDER BY with variables/parameters could also be interesting. Assuming we are using the same naive translation we use for numeric and Boolean constants, SQL Server will throw:

  • For non-numeric constants:

    A constant expression was encountered in the ORDER BY list, position 1.

  • For variables:

    The SELECT item identified by the ORDER BY number 2 contains a variable as part of the expression identifying a column position. Variables are only allowed when ordering by an expression referencing a column name.

It seems that the general approach could be to swallow any elements in the ORDER BY list that are either variables or constants to prevent generating SQL that would cause an exception (in the variable case and for non-numeric constants) or results ordered by some arbitrary column (in the numeric constant case). That said there are cases that are a bit more complicated that will still cause an exception.

It is true if there are paging operations and we remove all elements of the ORDER BY list, we will have to introduce a dummy ORDER BY. Maybe that can be handled by the existing logic that introduces ORDER BY @@ROWCOUNT?

(BTW if anyone finds anything other than @@ROWCOUNT that still works and produces stable ordering that would be great. I almost prefer @@SPID to @@ROWCOUNT because the latter can give you the false impression that it represents something in the query)

@rowanmiller rowanmiller added this to the Backlog milestone Jul 25, 2016
@kunnis
Copy link

kunnis commented Sep 16, 2016

@divega Have you considered ORDER BY (SELECT 1)?

@divega
Copy link
Contributor

divega commented Sep 16, 2016

@kunnis actually no. Great tip!

@smitpatel @anpete this seems to works. I think we should consider doing it here as well as when we need an arbitrary ORDER BY for paging. Thoughts?

@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 16, 2016

Won't work with SQL ce..,

@divega divega removed this from the Backlog milestone Sep 16, 2016
@anpete
Copy link
Contributor

anpete commented Sep 17, 2016

@divega Yes. I recently also discovered this trick when working on the improved GroupJoin prototype.

@divega
Copy link
Contributor

divega commented Sep 17, 2016

@ErikEJ does SQL CE have all the dame issues? E.g.is it already broken for these scenarios?

@kunnis
Copy link

kunnis commented Sep 17, 2016

@divega I think it currently has the same problem.

https://github.com/ErikEJ/EntityFramework.SqlServerCompact/blob/master/src/Provider40/Query/Sql/Internal/SqlCeQuerySqlGenerator.cs#L76 Uses ORDER BY GETDATE() to get around the fact that the Order By clause is required.

CE doesn't allow for a scalar_subquery (which is what (select 1) is) in the Order By (See https://technet.microsoft.com/en-us/library/ms174501(v=sql.110).aspx ) where MSSQL does (See https://msdn.microsoft.com/en-us/library/ms190286.aspx )

I setup SqlCE earlier today and ran a statement ending in ORDER BY @@rowcount. That threw an exception that @@rowcount wasn't allowed there. @@spid doesn't work either. I tried several different variations and I couldn't find a clause that was clear about not doing anything and worked in both MSSQL and SqlCE.

@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 17, 2016

@divega - what @kunnis says (thanks)

@smitpatel
Copy link
Contributor Author

The same issue persists in SQLite too. Fixing it for relational provider in general.

@smitpatel smitpatel changed the title SQL Server: OrderBy constant generates incorrect query for integer/boolean Query: OrderBy constant generates incorrect query for integer/boolean Sep 30, 2016
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Oct 15, 2022
@ajcvickers ajcvickers modified the milestones: 1.1.0-preview1, 1.1.0 Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

No branches or pull requests

7 participants