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

EF Core 7 - FirstWithoutOrderByAndFilterWarning in simple query with multiple aggregate functions #29782

Closed
raffaele-cappelli opened this issue Dec 6, 2022 · 5 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@raffaele-cappelli
Copy link

In order to perform a simple query with multiple aggregate functions in the select, such as:

select SUM(Value1), SUM(Value2)
from [Table]

I am using "group by (constant value)":

var r = (from x in db.Table
          group x by 1 into g
          select new
          {
            Sum1 = g.Sum(x => x.Value1),
            Sum2 = g.Sum(x => x.Value2),
          }).First();

I noted that in EF7 this produces warning CoreEventId.FirstWithoutOrderByAndFilterWarning ("The query uses the 'First'/'FirstOrDefault' operator without 'OrderBy' and filter operators. ...").

Questions

  • Is this the correct way to perform such queries in EF7, or is there another way that produces better SQL (see below for a complete example with the SQL generated by EF7)?
  • Shouldn't the warning be avoided in these cases (grouping on a constant value), since there will be at most one record in the result?
  • Currently, in order to avoid the warning, is there a better way, other than adding "orderby 1" ?

Example console app

using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;

using var db = new TestDb();

var qry = from x in db.Table
          group x by 1 into g
          select new
          {
            Sum1 = g.Sum(x => x.Value1),
            Sum2 = g.Sum(x => x.Value2),
          };

Console.WriteLine(qry.ToQueryString());

var result = await qry.FirstAsync();

Console.WriteLine($"{result.Sum1} {result.Sum2}");

class Table
{
  public int Id { get; set; }
  public int Value1 { get; set; }
  public int Value2 { get; set; }
}

class TestDb : DbContext
{
  public DbSet<Table> Table { get; set; }
  protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
  {
    optionsBuilder.UseSqlServer("Server=localhost;Database=Test;Trusted_Connection=True;Encrypt=False");
    optionsBuilder.LogTo(Console.WriteLine, LogLevel.Warning);
  }
}

Output of the example app

SELECT COALESCE(SUM([t0].[Value1]), 0) AS [Sum1], COALESCE(SUM([t0].[Value2]), 0) AS [Sum2]
FROM (
    SELECT [t].[Value1], [t].[Value2], 1 AS [Key]
    FROM [Table] AS [t]
) AS [t0]
GROUP BY [t0].[Key]
warn: 06/12/2022 09:00:12.252 CoreEventId.FirstWithoutOrderByAndFilterWarning[10103] (Microsoft.EntityFrameworkCore.Query)
      The query uses the 'First'/'FirstOrDefault' operator without 'OrderBy' and filter operators. This may lead to unpredictable results.
6 60

Provider and version information

EF Core version: 7.0
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 7.0

@ajcvickers
Copy link
Contributor

/cc @roji @maumar

1 similar comment
@ajcvickers
Copy link
Contributor

/cc @roji @maumar

@raffaele-cappelli
Copy link
Author

Hello, any news on this?

/cc @roji @maumar

@roji
Copy link
Member

roji commented Jan 5, 2023

Sorry for taking so long to look at this.

AFAIK, the pattern of using group-by-constant is indeed the only way to achieve querying the results of multiple aggregate operators. We've had discussions on a better API to allow expressing this more elegantly, but as of now that's the recommend way (unless I'm missing something).

As for the warning, I'd avoid it by simply using Single rather than First; that also conveys what you're doing better, clearly saying you expect just one result (rather than saying you want the first). This would change the SQL to have TOP(2) instead of TOP(1) , but there shouldn't be any perf degradation associated with that (and seems better than an order by 1).

As for the warning, technically it's true that we shouldn't warn, but identifying the precise pattern where the warning shouldn't be issued may be non-trivial; as the value is generally very low and Single seems more suited to this task, I don't see us working on this any time soon...

Let me know if this answers your questions and don't hesitate to ask for more clarifications.

@raffaele-cappelli
Copy link
Author

Thank you, I hadn't considered using .Single(): I agree it's better, both in terms of readability and (probably) performance. I will do that.

@ajcvickers ajcvickers added the closed-no-further-action The issue is closed and no further action is planned. label Jan 12, 2023
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

3 participants