Efcore: 3.0 regression: Produces invalid SQL for complex query

[joakimriedel]

After upgrading to 3.0.0-rc2 daily builds from 2.2.6, a query previously working is now generating invalid SQL. It's hard for me to make a small reproducible example since it renders into a complex query with lots of columns and joins (292 lines long..), but this is an outline of the resulting query:

SELECT [t14].[lots of columns..], [t21].[lots of columns..], [t28].[lots of columns..]
FROM (
    SELECT TOP(1) [lots of columns..]
    FROM [a table]
    INNER JOIN [other tables..]
    LEFT JOIN [**The Missing Table**] AS [t] ON [...]
    --- here is the table joined in as [t]
    LEFT JOIN [other tables..]
    WHERE [several filters]
    ORDER BY [a column]
) AS [t14]
OUTER APPLY (
    SELECT [t].[a column], [...]
    --- this is the error, it tries to reach [t] in this outer apply
) AS [t21]
OUTER APPLY (
    [other things]
) AS [t28]
LEFT JOIN [...]
[...]
ORDER BY [t14].[lots of columns..], [t21].[lots of columns..], [t28].[lots of columns..]

As you can see it tries to reach [t] from within outer apply but it's declared in another scope. How do I proceed to investigate this so that I can give you better details of this problem? I do not even know where to start debugging this to make EF Core produce a valid SQL. Perhaps this is something you know about and are already working on right now?

Further technical details

EF Core version: 3.0.0-rc2 daily builds
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET Core 3.0
Operating system: Win 10
IDE: Visual Studio 2019 preview

[smitpatel]

Most certainly it is a bug since generated SQL is invalid. In order for us to investigate, we need to get original LINQ query. Perhaps you can use TagWith to isolate which linq generated above SQL.

[joakimriedel]

@smitpatel I'd be happy to share the details, is there any way to send them a bit more private?

[smitpatel]

You can email it to me at "smpatel" at this place I work--microsoft.com

[joakimriedel]

@smitpatel email sent!

[smitpatel]

@jcemoller - Received. Give me few days to investigate. It is a massive query. 😆

[joakimriedel]

@smitpatel massive and critical, been in production well over a year now. I'll work on my side trying to break it down into pieces easier to chew and see if I can find anything of use. Thanks for investigating this for us!

[smitpatel]

Since you had expression tree for query already, can you get the output of new ExpressionPrinter().Print(query)? ExpressionPrinter is public class available in EF Core 3 to print out expression trees in more readable format.

[joakimriedel]

@smitpatel sent to your inbox!

[joakimriedel]

@smitpatel after hours of debugging, I think I've got something.

This does not work

```c#
await query.FirstOrDefaultAsync()

# This works! 🎉

```c#
(await query.ToListAsync()).FirstOrDefault()

There's a LOT of data unneccessarily flowing from the SQL server, but at least the generated SQL is now valid.

(same error with SingleOrDefaultAsync())

[Shane32]

Might want to try (await query.Take(1).ToListAsync()).FirstOrDefault() ... if it works, it would be a good temporary fix.

[joakimriedel]

@Shane32 thanks, would've made a nice workaround, but unfortunately it renders the same invalid sql as SingleOrDefaultAsync or FirstOrDefaultAsync.

[smitpatel]

@jcemoller - Thanks for all the details you sent over email. I am still processing it. The conversation happening here made me realize where is the bug. I am still to figure out a repro and investigate how to fix but the root cause to avoid the issue is around having Take on the top level query for your case. Hence doing FirstOrDefault fails (that put .Take(1)) & Take also fails. An efficient work-around (untested code)
C# ResultType result = null; await foreach (var element in query.AsAsyncEnumerable()) { result = element; break; }
This would avoid allocating whole list for all results and not generate invalid SQL.

[joakimriedel]

Thanks @smitpatel , I will try this work-around next week. Just wanted to let you know that I have now worked out all (__*__) the migration issues - and this week we're finally live with 3.0 code in production! Preliminary results look promising, we're seeing on average http request times going down from 70 ms to 54 ms with net/ef core 3.0 compared to 2.2.6! Great work!

__*__ I had to skip a single integration test due to some regression in memory provider, will post issue later when I got time to package a reproducible example.

[smitpatel]

@jcemoller - I have merged a fix for similar kind of issue. In couple of days it should be available in nightly builds. Can you run your scenario against nightly build to see if it is fixed?

[joakimriedel]

Thanks @smitpatel, just verified with _3.1.0-preview3.19554.8_ but unfortunately the generated SQL is still invalid. I made a diff of the resulting SQL between preview1 and preview3, and the only visible change was related to null checks which seems a bit relaxed in preview3.

[smitpatel]

@jcemoller - Thanks for verifying. I will investigate this further.

[joakimriedel]

@smitpatel just sent email with a small repro for triggering the invalid SQL code

[joakimriedel]

@smitpatel did the repro help or do you need further info?

[smitpatel]

@joakimriedel - I haven't had chance to debug into the repro. Though I have this issue on mind while working on other. We fixed bunch of issue with Skip/Take and navigations. #19825 recently went in which could cause similar issue. I believe due to complexity of query, there could be quite a few issues combined together. Few other issues I can think of which would contribute to this is #17337, #19763 . I hope to get this one working in 5.0 timeframe.

[joakimriedel]

@smitpatel thanks for the update! I appreciate you looking into this due to being a regression from 2.2 that affected several queries for us in a negative way.

[joakimriedel]

Hey EF Core team, what's the progress here? I see you're making loads of great new features on 5.0, but what about these small query bugs that you stumble upon every time a project goes moderately complex.

This case is about a regression from 2.2 to 3.0 where EF Core started generating invalid SQL on a particular complex query where I use FirstOrDefault() to retrieve the item. Workaround for 3.0 was to use ToList() to query all items (and then picking the first item locally), but this is sending unneccessary data to the client and possibly not generating the optimal query plan in SQL Server.

It did not get fixed in 3.0, not in 3.1, could I hope for 5.0?

ping @smitpatel

[ajcvickers]

@joakimriedel We discussed again, but the repro and the query here are quite complex and may require significant investigation time. There's a small chance we may be able to pull this into EF Core 5.0, otherwise we will get to it in EF Core 6.0.

[smitpatel]

@joakimriedel - I investigated in the details from the email. The last repro did help
I isolated 2 issues

• #22222 - Incorrect alias used for join inside Outer apply
• #22223 - Column used from outer in Outer apply subquery not projected.

Based on SQL you send for original issue, #22223 looks close enough (that is the column is not projected in outer query, though the alias is also incorrect as the alias used is already pushed down deep inside subquery. I will work on fixing those isolated issues.

[smitpatel]

We cannot fix any of these in 5.0 yet. We will revisit this in 6.0

[joakimriedel]

Thanks for the investigation @smitpatel I'm delighted to hear that you've managed to find the culprit.

How far away is 6.0 @ajcvickers ? This query got broken upgrading from 2.2 to 3.0, and it seems a long way to go with 6.0 before resolving the regression. I really understand that you have a lot of work with 5.0 seeing the issue tracker, but would generally prefer having regression bugs prioritized before adding any new features.

[joakimriedel]

@smitpatel with the latest RC1 bits I'm able to run this query without any workaround. The issues you found might still apply however, since I did some refactoring on the dto objects earlier this spring and I think they might now be "unnested" one level less deep now than when I originally posted this. But still, it's great progress and I'm happy not having to load all items to the client any more on this particular query in our application! 🎉

[smitpatel]

@joakimriedel - Should we close this issue or are there any pending issues (apart from I already filed) where you can provide some code to repro?

[joakimriedel]

@smitpatel let's close this for now!

[smitpatel]

@ajcvickers - Can you re-arrange labels here?