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

LEAST/GREATEST nullability work #32458

Merged
merged 1 commit into from
Jul 23, 2024
Merged

Conversation

roji
Copy link
Member

@roji roji commented Nov 30, 2023

@maumar tiny fixup PR; if EF.Functions.Greatest/Least is used over a nullable value type, that trips up the debug assertion (SqlExpression nodes can't have a nullable value type). So this unwraps that before creating the function.

Fixup to #32338

maumar
maumar previously approved these changes Nov 30, 2023
@roji
Copy link
Member Author

roji commented Dec 2, 2023

Turning this into a draft, there are unfortunately some other nullability-related aspects of this that need to be taken care of...

@roji roji marked this pull request as draft December 2, 2023 08:50
@maumar maumar self-requested a review December 20, 2023 22:08
@roji roji force-pushed the NullableLeastGreatest branch from 594a31d to f1181d9 Compare July 22, 2024 09:33
@roji roji dismissed maumar’s stale review July 22, 2024 09:34

PR has been redone and needs a new review

@roji roji mentioned this pull request Jul 22, 2024
45 tasks
@roji roji force-pushed the NullableLeastGreatest branch from f1181d9 to 2fc6828 Compare July 22, 2024 09:44
@roji
Copy link
Member Author

roji commented Jul 22, 2024

Nullability analysis

There are two diverging behaviors across databases with respect to NULL arguments (there's a reason I procrastinated on this work...):

  • PostgreSQL and SQL Server ignore NULLs - GREATEST/LEAST only return NULL if all arguments evaluate to NULL. In that sense the behavior is similar to COALESCE.
  • SQLite, MySQL and MariaDB propagate NULLs - NULL is returned if any argument evaluates to NULL (more standard behavior).

We have three cases where we translate to GREATEST/LEAST:

  • Math.Max/Min. These have a restricted set of overloads over numeric CLR types - only non-nullable - so it's OK.
  • Queryable/Enumerable.Max/Min. These do have overloads for nullable int, etc., where null is returned when all arguments are null (or there are no arguments); in other words, the PG/SQL Server behavior. That means we can use GREATEST/LEAST for all these overloads on SQL SERVER/PG, but for the other databases we can only use them for the non-nullable overloads.
  • Our own EF.Functions.Greatest/Least (for any type). Here, as usual with EF.Functions translations, we just translate to the database function, so the behavior will be different across databases with respect to nullability.

Changes in this PR

  • Generation of GREATEST/LEAST moved from SqlExpressionFactory to RelationalSqlTranslatingExpressionVisitor, defaulting to no translation (forcing the provider to implement). This was done because:
    • The functions aren't always supported (e.g. older SQL Server), and everything on SqlExpressionFactory is currently always supported (see API review comment).
    • Even when supported, the diverging NULL propagation behavior means we want each provider to supply their own implementation with the correct nullability propagation etc.
  • Only translate {Queryable,Enumerable.{Max/Min} to GREATEST/LEAST if the provider's nullability propagation matches .NET (i.e. return null only if all arguments evaluate to null), or if the elements are non-nullable.
  • Generally better factoring.

@roji roji marked this pull request as ready for review July 22, 2024 09:44
@roji roji requested a review from a team July 22, 2024 09:45
@roji
Copy link
Member Author

roji commented Jul 22, 2024

@dotnet/efteam I've brought this back and done the necessary fixed, see above.

@roji roji changed the title Properly support LEAST/GREATEST over nullable value types LEAST/GREATEST nullability work Jul 22, 2024
@roji roji force-pushed the NullableLeastGreatest branch from 2fc6828 to 4e3efed Compare July 22, 2024 10:54
@roji roji force-pushed the NullableLeastGreatest branch from 4e3efed to e70041d Compare July 22, 2024 11:11
@roji roji merged commit 6e9e287 into dotnet:main Jul 23, 2024
7 checks passed
@roji roji deleted the NullableLeastGreatest branch July 23, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants