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

SQL Server: Translate Math.Max/Min in non-aggregate context #27794

Closed
yesmey opened this issue Apr 10, 2022 · 9 comments · Fixed by #32338
Closed

SQL Server: Translate Math.Max/Min in non-aggregate context #27794

yesmey opened this issue Apr 10, 2022 · 9 comments · Fixed by #32338
Assignees
Labels
area-query area-sqlserver closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement

Comments

@yesmey
Copy link
Contributor

yesmey commented Apr 10, 2022

Today Math.Max / Math.Min is not being translated for SQL Server - only for sqlite as introduced in #10533

Transact-SQL does not have an MIN or MAX function, so there's no 1-to-1 mapping, but it is however possible to translate it into a CASE WHEN statement.
For example

Math.Max(Column1, Column2)

is equivalent to

CASE
    WHEN Column1 >= Column2 THEN Column1
    ELSE Column2
END

As a suggestion, I made a reference implementation by adding the following translation to SqlServerMathTranslator.cs

For Math.Max it looks like this:

var left = arguments[0];
var right = arguments[1];

var typeMapping = ExpressionExtensions.InferTypeMapping(left, right);
left = _sqlExpressionFactory.ApplyTypeMapping(left, typeMapping);
right = _sqlExpressionFactory.ApplyTypeMapping(right, typeMapping);

return _sqlExpressionFactory.Case(
    new[] { new CaseWhenClause(_sqlExpressionFactory.GreaterThanOrEqual(left, right), left) },
    right);

and Math.Min:

// same type mapping...
return _sqlExpressionFactory.Case(
    new[] { new CaseWhenClause(_sqlExpressionFactory.LessThanOrEqual(left, right), left) },
    right);

Link to my branch of the example

@AraHaan
Copy link
Member

AraHaan commented Apr 10, 2022

I though TSQL did have an Min and Max operation, but are Aggregate functions that can be used in normal queries in a special way.

@yesmey
Copy link
Contributor Author

yesmey commented Apr 10, 2022

Oh yes, I meant that no scalar version of that function exists

@roji
Copy link
Member

roji commented Apr 11, 2022

SQL Server is introducing the GREATEST and LEAST functions, which are standard across relational databases (see announcements). These are already available on Azure SQL, and will apparently be available in the next version of on-premise SQL Server.

So while they aren't generally available (except on Azure), I'm not sure it makes sense to introduce the CASE/WHEN translation proposed above (we'll probably want to add GREATEST/LEAST translations when they're available). Note that it should be easy to do the above mapping with user-defined function mapping.

@yesmey
Copy link
Contributor Author

yesmey commented Apr 11, 2022

@roji I hadn't heard about this, that's great! Yes, my proposal more of a hack, it makes sense if you'd want to wait for those to come avaliable instead

@AraHaan
Copy link
Member

AraHaan commented Apr 11, 2022

SQL Server is introducing the GREATEST and LEAST functions, which are standard across relational databases (see announcements). These are already available on Azure SQL, and will apparently be available in the next version of on-premise SQL Server.

So while they aren't generally available (except on Azure), I'm not sure it makes sense to introduce the CASE/WHEN translation proposed above (we'll probably want to add GREATEST/LEAST translations when they're available). Note that it should be easy to do the above mapping with user-defined function mapping.

Wait so SQL Server 2022 is a thing now?

@roji
Copy link
Member

roji commented Apr 11, 2022

It will certainly be a thing at some point...

@MKatGH
Copy link

MKatGH commented Jul 29, 2022

Am I missing some deeper logic? The code below would do just that 'out of the box' in ef.core.

public class Demo
{
    public int Value1 { get; set; }
    public int Value2 { get; set; }
}

var r = this.Context.DemoRecords
    .Select(e => new
    {
        MinValue = e.Value1 > e.Value2 ? e.Value2 : e.Value1,    // Like Math.Min(e.Value1, e.Value2)
        MaxValue = e.Value1 > e.Value2 ? e.Value1 : e.Value2    // Like Math.Max(e.Value1, e.Value2)
    })
    .ToList();

Ok, I'll give you that 'Math.Max(Column1, Column2)' is a little bit shorter, but not by that much.
Support for GREATEST and LEAST would be better, since that will support multiple arguments. Math.Max is limited to two.

@yesmey
Copy link
Contributor Author

yesmey commented Jul 29, 2022

@MKatGH you're correct, it would just be for convenience purposes. string.IsNullOrEmpty for example is also trivial to express out of the box, but there is a translator for that. I don't really have a strong opinion for or against it, it's just a mere suggestion

@roji roji added the blocked label Jul 29, 2022
@roji roji removed the blocked label Sep 11, 2023
@roji
Copy link
Member

roji commented Sep 11, 2023

Note also #31681 which is for translating to GREATEST/LEAST.

@roji roji self-assigned this Nov 18, 2023
roji added a commit to roji/efcore that referenced this issue Nov 18, 2023
@roji roji changed the title SQL Server: Translate Math.Max and Math.Min SQL Server: Translate Math.Max/Min in non-aggregate context Nov 18, 2023
roji added a commit to roji/efcore that referenced this issue Nov 18, 2023
roji added a commit to roji/efcore that referenced this issue Nov 18, 2023
roji added a commit to roji/efcore that referenced this issue Nov 18, 2023
roji added a commit to roji/efcore that referenced this issue Nov 18, 2023
roji added a commit to roji/efcore that referenced this issue Nov 27, 2023
roji added a commit that referenced this issue Nov 28, 2023
@roji roji added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Nov 28, 2023
@roji roji modified the milestones: Backlog, 9.0.0 Nov 28, 2023
@ajcvickers ajcvickers modified the milestones: 9.0.0, 9.0.0-preview1 Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query area-sqlserver closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants