Query: Issue warning for queries with paging operations (First, Take, Skip) without OrderBy

[ericvb]

Steps to reproduce

Download the source from the Automapper project (https://github.com/AutoMapper/AutoMapper).
Take the unittest ExpandCollections.cs from the project IntegrationTests.Net4. It is working with EF 6.
See the expressions and the sql generated.
Add the project IntegrationTests.NetCore from the attachment zip. It is the same project, but now for EF core.
IntegrationTests.NetCore.zip

I sligthly adapted the original unit test.
ExpandCollectionsEric.zip

The issue

For EF 6 we see one generated SQL query.

SELECT
    [Project1].[CourseId] AS[CourseId], 
    [Project1].[CourseName] AS[CourseName], 
    [Project1].[C3] AS[C1], 
    [Project1].[ContentId] AS[ContentId], 
    [Project1].[C1] AS[C2], 
    [Project1].[CategoryName] AS[CategoryName], 
    [Project1].[ShortCategory_CategoryId] AS[ShortCategory_CategoryId], 
    [Project1].[C2] AS[C3], 
    [Project1].[CategoryName1] AS[CategoryName1], 
    [Project1].[Category_CategoryId] AS[Category_CategoryId], 
    [Project1].[ContentName] AS[ContentName]
FROM(SELECT
                 [Limit1].[CourseId] AS[CourseId],
                 [Limit1].[CourseName] AS[CourseName],
                 [Join2].[ContentId] AS[ContentId],
                 [Join2].[ContentName] AS[ContentName],
                 [Join2].[Category_CategoryId] AS[Category_CategoryId],
                 [Join2].[ShortCategory_CategoryId] AS[ShortCategory_CategoryId],
                 [Join2].[CategoryName1] AS[CategoryName],
                 [Join2].[CategoryName2] AS[CategoryName1],
                 CASE WHEN ([Join2].[ContentId] IS NULL) THEN CAST(NULL AS int) WHEN([Join2].[CategoryId1] IS NOT NULL) THEN 1 END AS[C1],
                CASE WHEN([Join2].[ContentId] IS NULL) THEN CAST(NULL AS int) WHEN([Join2].[CategoryId2] IS NOT NULL) THEN 1 END AS[C2],
                CASE WHEN([Join2].[ContentId] IS NULL) THEN CAST(NULL AS int) ELSE 1 END AS[C3]
                        FROM(SELECT TOP (1) [Extent1].[CourseId]
                        AS[CourseId], [Extent1].[CourseName]
                        AS[CourseName]
                FROM[dbo].[TrainingCourses]
                        AS[Extent1]
                WHERE N'Course 1' = [Extent1].[CourseName] ) AS[Limit1]
                LEFT OUTER JOIN(SELECT[Extent2].[ContentId] AS[ContentId], [Extent2].[ContentName] AS[ContentName], [Extent2].[Category_CategoryId] AS[Category_CategoryId], [Extent2].[ShortCategory_CategoryId] AS[ShortCategory_CategoryId], [Extent2].[TrainingCourse_CourseId] AS[TrainingCourse_CourseId], [Extent3].[CategoryId] AS[CategoryId1], [Extent3].[CategoryName] AS[CategoryName1], [Extent4].[CategoryId] AS[CategoryId2], [Extent4].[CategoryName] AS [CategoryName2]
                FROM   [dbo].[TrainingContents] AS [Extent2]
                LEFT OUTER JOIN [dbo].[Categories] AS[Extent3] ON [Extent2].[ShortCategory_CategoryId] = [Extent3].[CategoryId]

                LEFT OUTER JOIN[dbo].[Categories] AS[Extent4] ON [Extent2].[Category_CategoryId] = [Extent4].[CategoryId]) AS[Join2] ON[Limit1].[CourseId] = [Join2].[TrainingCourse_CourseId]
                    )  AS[Project1]
                    ORDER BY[Project1].[CourseId]
                        ASC, [Project1].[C3]
                        ASC

For EF core we see 3 generated SQL queries:

                SELECT TOP(1)[dto].[CourseId], [dto].[CourseName]
                        FROM[TrainingCourses] AS[dto]
                WHERE[dto].[CourseName] = N'Course 1'

                SELECT[dto.ShortCategory].[CategoryId], [dto.ShortCategory].[CategoryName]
                        FROM[Categories] AS[dto.ShortCategory]

                exec sp_executesql N'SELECT [dto.Category].[CategoryId], [dto.Category].[CategoryName], [dto0].[ShortCategoryCategoryId], CASE
                    WHEN[dto0].[ShortCategoryCategoryId]
                        IS NULL
                    THEN CAST(0 AS BIT) ELSE CAST(1 AS BIT)
                END, CASE
                    WHEN[dto0].[CategoryId]
                        IS NULL
                    THEN CAST(0 AS BIT) ELSE CAST(1 AS BIT)
                END, [dto0].[ContentName], [dto0].[ContentId]
                        FROM[TrainingContents] AS[dto0]
                LEFT JOIN[Categories] AS[dto.Category] ON[dto0].[CategoryId] = [dto.Category].[CategoryId]
                        WHERE @_outer_CourseId = [dto0].[TrainingCourseCourseId]
                ORDER BY[dto0].[CategoryId]',N'@_outer_CourseId int',@_outer_CourseId=1

We can discuss about why 3 queries, but because we have yet basic LINQ support in this version of EF core, I hope that this will be just a starting situation (why a select top1 and a full table scan for the Category table?)

My point of concern is here the SELECT TOP(1) without an ORDER BY.
Normally that is always a bug, very difficult to find if there is always only one record expected.
There exists no default order in ANSI SQL standard or in SQL Server.
A SELECT TOP 1 without an ORDER BY will give a random row from the set. Which one is depending from the resource use of the sql server at that moment.
Probably it is a check to verify if there is one record, but it is still a bug...

[maumar]

We don't want to add ordering for the user as it's sometimes not clear what the orderby column(s) should be. However we should issue a warning in the logger if we encounter a query (or subquery) with Take or Skip and no orderby

[maumar]

btw, we now produce more efficient queries for this scenario:

SELECT TOP(1) [tc].[CourseId], [tc].[CourseName]
FROM [TrainingCourses] AS [tc]
WHERE [tc].[CourseName] = N'Course 1'

and

SELECT [c.Category].[CategoryId], [c.Category].[CategoryName], [c].[ContentId], [c].[ContentName], [c.Category].[CategoryId]
FROM [TrainingContents] AS [c]
LEFT JOIN [Categories] AS [c.Category] ON [c].[CategoryId] = [c.Category].[CategoryId]
WHERE @_outer_CourseId = [c].[TrainingCourseCourseId]
ORDER BY [c].[CategoryId]

[maumar]

Fixed in 527a55f

[WhatFreshHellIsThis]

This is not working correctly with FromSQL. I do have an order by but still incorrectly get the warning :
=-=-=-
2019-01-18 09:00:25.5078|DEBUG|Microsoft.EntityFrameworkCore.Query|Compiling query model:
'(from Widget _4 in DbSet
select [_4])
.AsNoTracking()
.FromSql('SELECT *, xmin FROM AWIDGET ORDER BY ID DESC')
.Skip(__p_1)
.Take(__p_2)'
2019-01-18 09:00:25.5078|WARN|Microsoft.EntityFrameworkCore.Query|Query: '(from Widget _4 in DbSet select [_4]).Skip(__p_1).Take(__p_2)' uses a row limiting operation (Skip/Take) without OrderBy which may lead to unpredictable results.

=-=-=-=-

My end users are going to consistently see a pointless warning message in my application's log, how can I disable this warning?

[smitpatel]

@WhatFreshHellIsThis - The order by needs to be in LINQ query. Your ORDER BY is inside SQL. We don't parse SQL.

[WhatFreshHellIsThis]

Yes exactly which is why this functionality that was added is a broken feature. It is going to freak out my end users who will see that completely incorrect error inside the application log. This needs to be fixed, it's not implemented correctly.

[WhatFreshHellIsThis]

Or, is there a way to disable this warning if it can't be implemented correctly?

[smitpatel]

It is not broken feature. It follows design principle for us that we don't try to analyze user provided SQL. With that in mind it is implemented correctly. If you want to avoid warning then instead of putting order by inside your SQL, you can put it in your LINQ.

[WhatFreshHellIsThis]

OrderBy is part of a dynamic feature where a user selects which column they want to order a grid by, hence it's required to be in the dynamic sql.

How is it not a broken feature if it doesn't work correctly and reports incorrect facts?
Is it not possible to look into the provided sql for the presence of "order by" and turn off the warning?

[smitpatel]

Is it not possible to look into the provided sql for the presence of "order by" and turn off the warning?

Strictly no. Parsing user provided SQL is hard and prone to errors.

It is a warning not an error. You may see it as an error if you turn on warnings as errors. That being said, the whole purpose of it being warning is, if it is fine for you to run the query then go for it. We just warned you but it may be still correct results. The feature is not broken because it does report warning accurately in the cases, we should. It may report warning in some cases inaccurately but we cannot identify those cases any way. So there is nothing actionable.