Translate LongCount() on SqlServer #15774
Conversation
Includes refactoring that makes it easier for providers to override behavior. Fixes dotnet#15718
| @@ -456,6 +455,10 @@ protected override ShapedQueryExpression TranslateLongCount(ShapedQueryExpressio | |||
| return source; | |||
| } | |||
|
|
|||
| protected virtual SqlExpression GenerateLongCountExpression() | |||
| => SqlExpressionFactory.ApplyDefaultTypeMapping( | |||
| SqlExpressionFactory.Function("COUNT", new[] { SqlExpressionFactory.Fragment("*") }, typeof(long))); | |||
There was a problem hiding this comment.
Just asking provider to provide us the function name is much easier approach. And that could be applied to anything which translate to function. It also gives npgsql chance to generate lower case names if wanted.
There was a problem hiding this comment.
The reason I did this, is that in PostgreSQL COUNT() always returns BIGINT (kind of the opposite problem as with implementing LongCount on SqlServer). So I need to apply an additional cast down to INTEGER.
In the old pipeline this is actually done at the SQL generation step but that's pretty ugly/hacky. Allowing providers to return an expression in the MethodTranslatingExpressionVisitor seems flexible enough to allow for various provider customizations, without placing too much burden on the provider writer...
We could also have two layers of extension points (one for just the function name, another for the entire expression) but that seems like too much... If you don't like the way I did it I can always reimplement the same SQL generation hack on the new pipeline.
There was a problem hiding this comment.
I wasn't aware that postgre needed such thing. Let me re-think.
There was a problem hiding this comment.
By the way, I suspect customizations may also be needed for AVG - the current code seems to make some (SQL Server?) assumptions on what types AVG returns. On PostgreSQL AVG returns:
numeric for any integer-type argument, double precision for a floating-point argument, otherwise the same as the argument data type
So it may be necessary again for Npgsql to override that entire expression, but not the rest of TranslateAverage().
There was a problem hiding this comment.
SqlServer:
- Double precision for floats
- Matches argument type for others
SQLite
- Real (the only data type for floating point no) for any result.
C#
- Double for int/long
- Matches argument type for others
So for int/long we need to cast them in advance for accurate result (I am not sure what exactly numeric type in postgre is and how it maps to a double in C# so this may or may not needed in postgre)
For float since both the providers are doing it (even postgre), we need to cast it to float on server side after calculating average.
There was a problem hiding this comment.
Isn't the current approach in this PR enough? It allows provider to return any expression (so including casts or other things) but still keeps pushdown and other details in relational. Just a way to split the provider-specific bits from the non-provider-specific ones.
This isn't necessary urgent and I don't have super strong feelings about it - let me know what you prefer and I can do that.
There was a problem hiding this comment.
Current approach in PR does not look right.
There was a problem hiding this comment.
OK, no problem - for now I will simply allow providers to define the function names, and I'll do the same hack for PostgreSQL. Will update the PR tomorrow.
There was a problem hiding this comment.
Leave this issue for later unless you are fixing it the way it is done in old pipeline. (which may be throw away work so not necessary to fix right away)
There was a problem hiding this comment.
OK, will defer this for later then.
Includes refactoring that makes it easier for providers to override behavior.
Fixes #15718
@smitpatel if the approach is OK I can look into refactoring the other aggregate functions in a similar way (e.g. MAX()).