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

Cosmos: Min/Max/Average over nullable value type throws on no data #35094

Open
alexandrereyes opened this issue Nov 13, 2024 · 13 comments · May be fixed by #35173
Open

Cosmos: Min/Max/Average over nullable value type throws on no data #35094

alexandrereyes opened this issue Nov 13, 2024 · 13 comments · May be fixed by #35173

Comments

@alexandrereyes
Copy link

alexandrereyes commented Nov 13, 2024

The code below is throwing "sequence contains no elements", even though there is a conversion to null.

await Set<Order>().MinAsync(o => (decimal?)o.TotalPrice)

This is blocking me from migrating to EF Core 9, since the only workaround is very bad (make two queries, the other one to verify if there is some record).

EF Core version: 9
Database provider: Microsoft.EntityFrameworkCore.Cosmos
Target framework: NET 9.0
Operating system: Windows 11
IDE: Visual Studio 2022 17.12.0

@ajcvickers
Copy link
Member

Duplicate of #29447

@ajcvickers ajcvickers marked this as a duplicate of #29447 Nov 13, 2024
@ajcvickers
Copy link
Member

@roji Based on some discussions we had, this could be a place where following the relational behavior and returning null might make sense.

@roji
Copy link
Member

roji commented Nov 13, 2024

We could revisit this... AFAIK we simply follow the documented LINQ behavior here: Enumerable.Max throws for no elements, and the behavior here is clearly documented (source contains no elements).

A trick to do this currently without roundtrip is the following:

var max = await context.Blogs.GroupBy(b => 1).Select(g => g.Max(b => b.Id)).FirstOrDefaultAsync();

This selects out the maximum ID if there are any rows, and zero otherwise. The SQL here isn't ideal:

SELECT TOP(1) MAX([b0].[Id])
FROM (
    SELECT [b].[Id], 1 AS [Key]
    FROM [Blogs] AS [b]
) AS [b0]
GROUP BY [b0].[Key]

... but it probably doesn't actually perform differently than the simple one...

Am OK if we want to rediscuss this in design!

@roji
Copy link
Member

roji commented Nov 13, 2024

This is blocking me from migrating to EF Core 9

@alexandrereyes I'm not aware of a change in EF Core 9 here - which version are you upgrading from where this worked?

@ajcvickers
Copy link
Member

I don't feel strongly about it, it's just we had discussed this since the dupe issue was closed as by-design.

@alexandrereyes
Copy link
Author

alexandrereyes commented Nov 13, 2024

This is blocking me from migrating to EF Core 9

@alexandrereyes I'm not aware of a change in EF Core 9 here - which version are you upgrading from where this worked?

From Ef Core 8 with CosmosDb SQL, the difference is that EF Core 8 doesn't throw an exception if you add a (decimal?) in the instruction.
I can't use your workaround because ef core cosmosdb cannot translate the group by operation.
Using two selects to achieve this result is very inefficient....

@roji
Copy link
Member

roji commented Nov 13, 2024

@alexandrereyes no matter what we decide here, there won't be a change coming at least until EF 10, which is a year away (EF 9 was just released) - I hope GroupBy will be implemented for Cosmos at that point, and that it would be an acceptable workaround.

@alexandrereyes
Copy link
Author

alexandrereyes commented Nov 13, 2024

@roji This is a regression, I think that a lot of people will be impacted because it's a very basic operation

@roji
Copy link
Member

roji commented Nov 13, 2024

As I've asked you above, can you please explain how this is a regression? Which version of EF allowed this sort of query exactly?

@alexandrereyes
Copy link
Author

alexandrereyes commented Nov 13, 2024

On EF Core 8 Cosmos Db, the code below works:
Set<Order>().AverageAsync(a => (decimal?)a.TotalPrice)

On EF Core 9 Cosmos Db, the code below does not work, it throws sequence contains no elements exception:
Set<Order>().AverageAsync(a => (decimal?)a.TotalPrice)

On both frameworks, It should only throw the exception if I don't provide the nullable convertion, for example:
Set<Order>().AverageAsync(a => a.TotalPrice)

@roji
Copy link
Member

roji commented Nov 14, 2024

Apologies. I missed that this was about Cosmos specifically, and also had a mistaken idea of our behavior in relational.

@ajcvickers so it seems that in relational we actually don't throw for Min/Max/Average over the empty set, when the return value is nullable. In other words, in #29447 the user expected us to return e.g. int.MinValue for Max over non-nullable int, which we obviously don't do. But here the question is Max over nullable int.

The enumerable versions of Min/Max/Average also return null when the type is nullable, so our relational behavior is aligned with that:

int?[] ints = [];
Console.WriteLine(ints.Max() is null); // True

So I think we should indeed treat this as a regression for 8.0 and fix it... Does that sound right @ajcvickers? Can bring to design discussion if you think we need to.

Repro for Cosmos
await using var context = new BlogContext();
await context.Database.EnsureDeletedAsync();
await context.Database.EnsureCreatedAsync();

_ = await context.Sessions.AverageAsync(s => (int?)s.SomeInt);

public class BlogContext : DbContext
{
    public DbSet<Session> Sessions { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseCosmos(
                "https://192.168.64.6:8081",
                "C2y6yDjf5/R+ob0N8A7Cgv30VRDJIWEHLM+4QDU5DE2nQ9nDuVTqobD4b8mGGyPMbIZnqyMsEcaGQy67XIw/Jw==",
                "Test",
                o => o.HttpClientFactory(() => new HttpClient(
                        new HttpClientHandler
                        {
                            ServerCertificateCustomValidationCallback =
                                HttpClientHandler.DangerousAcceptAnyServerCertificateValidator
                        }))
                    .ConnectionMode(ConnectionMode.Gateway))
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();
}

public class Session
{
    public Guid Id { get; set; }
    public string PartitionKey { get; set; }

    public int SomeInt { get; set; }
}

@ajcvickers
Copy link
Member

@roji Sounds right to me.

@roji
Copy link
Member

roji commented Nov 14, 2024

This behavior seems to have been changed unintentionally (by me) in #34044:
https://github.com/dotnet/efcore/pull/34044/files#diff-28145bf59ab6b4afa190489d4065fc9de66e40eb4223a08e491635b217364408R538

The relevant tests are NorthwindAggregateOperatorsQueryCosmosTest.Max_no_data_nullable, Min_no_data_nullable, Average_no_data_nullable.

@roji roji self-assigned this Nov 14, 2024
@roji roji added this to the 10.0.0 milestone Nov 14, 2024
@roji roji changed the title Sequence contains no elements on MinAsync, MaxAsync, etc. Cosmos: Min/Max/Average over nullable value type throws on no data Nov 14, 2024
@roji roji assigned ajcvickers and unassigned roji Nov 14, 2024
ajcvickers added a commit that referenced this issue Nov 21, 2024
Fixes #35094

This was a regression resulting from the major Cosmos query refactoring that happened in EF9. In EF8, the functions Min, Max, and Average would return null if the return type was nullable or was cast to a nullable when the collection is empty. In EF9, this started throwing, which is correct for non-nullable types, but a regression for nullable types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment