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

Added Greatest, Least and NullIf DbFunctions #2866

Closed
wants to merge 1 commit into from

Conversation

cloudlucky
Copy link
Contributor

I added 3 new DbFunctions: Greatest, Least and NullIf.
Documentation: https://www.postgresql.org/docs/current/functions-conditional.html

About Greatest and Least functions, I tried with params but it didn't work well for all use cases, so I used method overloads.

I know that Math.Max and Math.Min generate Greatest and Least respectively but when you need to get the Greatest or Least number with 6 or 7 properties, the code is less readable

// With Math.Max
var result = await db.MyEntity.Where(x => Math.Max(Math.Max(Math.Max(x.Prop1, x.Prop2), x.Prop3), x.Prop4) > 42).ToListAsync();

// vs Greatest Db Function
var result = await db.MyEntity.Where(x => EF.Functions.Greatest(x.Prop1, x.Prop2, x.Prop3, x.Prop4) > 42).ToListAsync();

Also, Math.Max and Math.Min only works with numbers which was an issue when I needed to get the greatest or least DateTime.

@cloudlucky cloudlucky changed the title Add Greatest, Least and NullIf DbFunctions Added Greatest, Least and NullIf DbFunctions Sep 10, 2023
@roji
Copy link
Member

roji commented Sep 11, 2023

@cloudlucky these three functions are standard SQL ones, which are supported pretty much across all databases the EF supports. That means that they should be added in EF itself, rather than in the PG provider specifically - I've opened dotnet/efcore#31681 and dotnet/efcore#31682 to track that. Adding support there should be similar to the work you've done.

However, I'd be interested in what difficulties you ran into when trying to translate variadic functions (with params), since adding all these overloads for different param numbers isn't great.

@cloudlucky
Copy link
Contributor Author

@roji Perfect, I will give it a shot to implement these in EF itself.

Also, about all these overloads, I agree with you, it's not great. In fact, my actual implementation is using params.
Note: I've only referred to Greatest Function in the following examples for brevity, but the same also applies to Least.

I currently have this signature where there is one required parameter.

public static T Greatest<T>(this DbFunctions _, T value, params T[] values)
    => throw new InvalidOperationException(CoreStrings.FunctionOnClient(nameof(Greatest)));

The generated SQL wasn't good as the params parameter created an array parameter in the SQL

... GREATEST(value, ARRAY[ values[0], values[1], values[2] ]) ...

But what we need is to pass each element of the array as a parameter to the function

... GREATEST(value, values[0], values[1], values[2]) ...

To achieve this, I created a method to extract SqlExpression in the Translator.
It takes the SqlExpression of the params parameter, extract the inner SqlExpressions and return them.

private static IEnumerable<SqlExpression> ConvertParamsToArguments(SqlExpression sqlExpression)
{
    if (sqlExpression is SqlConstantExpression sqlConstantExpression)
    {
        if (sqlConstantExpression.Value is not null && sqlConstantExpression is IEnumerable collection)
        {
            foreach (SqlExpression item in collection)
            {
                yield return item;
            }
        }
    }
    else if (sqlExpression is PostgresNewArrayExpression newArrayExpression)
    {
        foreach (var item in newArrayExpression.Expressions)
        {
            yield return item;
        }
    }

    throw new NotSupportedException("The values (params) parameter was created outside of the Expression which is currently not supported");
}

It works well but seems like a bit of a hack to me and not sure that is the best way to do this.

Except, it doesn't work for one specific scenario. If you declare an array outside of the Expression and then use it as the params parameter. e.g.:

var param = new[] { 1 };
var orderDetail = .... EF.Functions.Greatest(1, param) .....;

In that specific case, it creates a SqlParameterExpression and I'm not sure what to do. This is why there is a throw NotSupportedException.

So, I'm not sure what to do with this. Do you have any advice?

@roji
Copy link
Member

roji commented Sep 13, 2023

Yes, handling an array parameter does require some special handling. Note that PostgresNewArrayExpression is PG-specific, and won't be available in the EF relational layer (where Greater/Least should be implemented). Unless I'm mistaken, that means that you also can't implement the translation in a regular MethodTranslator (as you've done in this PR), since the MethodTranslator infrastructure requires all arguments to be translatable, which isn't the case here. In other words, you'll have to perform the translation from RelationalSqlTranslatingExpressionVisitor.VisitMethodCall (which is also the thing that calls MethodTranslators at the bottom), e.g. by adding and calling something like TryTranslateGreatestLeast. At that point you have access to the pre-translation, non-SQL expression and can do what you want.

Except, it doesn't work for one specific scenario. If you declare an array outside of the Expression and then use it as the params parameter.

EF has the [NonParameterized] attribute for this purpose; if you put it on a parameter (the values array in this case), you no longer get a parameter expression. Take a look at usages of this in the EF source code.

If you want, I'd suggest first doing a PR for NullIf in relational - which should be quite simple - and then tackling Greater/Least separately, as these will be more complex. For now, I'll go ahead and close this PG-specific PR - but feel free to post back if you want to discuss further.

@roji roji closed this Sep 13, 2023
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.

2 participants