Reduce cast usage for COUNT aggregate and add support for Mssql count_big #2061

maca88 · 2019-03-15T20:32:18Z

This is a continuation of #2036, which extends the cast reduce usage for count aggregate function. In addition, this PR introduces count_big sql function that is used when using LongCount and MsSql2000Dialect overrides it to use the specific Sql Server count_big function (by default count is used).

src/NHibernate/Linq/Visitors/HqlGeneratorExpressionVisitor.cs

maca88 · 2020-02-21T22:01:26Z

Rebased and renamed to ISQLFunction extension interface as it will be used in all ISQLFunction implementations which I will address in a separate PR. Initially, I planned to deprecate ISQLFunction.ReturnType in favor of the newly added ISQLFunctionExtended.GetEffectiveReturnType method but it turned out that it would produce an unwanted breaking change. For example the following test would fail in Sql Server:

nhibernate-core/src/NHibernate.Test/QueryTest/CountFixture.cs

Lines 23 to 24 in d4206e2

    
           object count = s.CreateQuery("select count(*) from Simple").UniqueResult(); 
        
           Assert.IsTrue(count is Int64);

because count effective return type in Sql Server is an int. So the idea is that ISQLFunction.ReturnType should return the same type no matter what database is used, which is basically what it does today, except for some functions which IMO should be corrected:

length: in Firebird and Oracle dialects returns Int64, SysbaseASE9 returns string (very suspicious) and in other dialect returns Int32.
mod: in Firebird dialect returns Double where in other dialects returns Int32

The intention of ISQLFunctionExtended.GetEffectiveReturnType method is be used by higher level api like linq in order to determine whether a cast or a transparent cast is needed.

src/NHibernate/Dialect/Function/ISQLFunction.cs

fredericDelaporte · 2020-03-29T17:54:24Z

src/NHibernate/Dialect/MsSql2000Dialect.cs

@@ -286,7 +286,8 @@ protected virtual void RegisterKeywords()

 		protected virtual void RegisterFunctions()
 		{
-			RegisterFunction("count", new CountBigQueryFunction());
+			RegisterFunction("count", new CountQueryFunction());
+			RegisterFunction("count_big", new CountBigQueryFunction());


Isn't this still causing the breaking change you were writing about?

For example the following test would fail in Sql Server:

nhibernate-core/src/NHibernate.Test/QueryTest/CountFixture.cs

Lines 23 to 24 in d4206e2

object count = s.CreateQuery("select count(*) from Simple").UniqueResult();

Assert.IsTrue(count is Int64);

because count effective return type in Sql Server is an int.

This changes the registration of HQL count for SQL-Server to map to SQL-Server count instead of count_big, which returns an int32 and fails if the count is bigger.

Isn't this still causing the breaking change you were writing about?

For HQL nothing is changed, the new GetEffectiveReturnType method is used only for Linq queries. CountQueryFunction.ReturnType still returns Int64, which is what HQL uses to determine the type, so hql queries using count will always be casted to Int64.

fredericDelaporte · 2020-03-29T17:58:46Z

src/NHibernate/Hql/Ast/ANTLR/SqlGenerator.g

@@ -150,7 +150,7 @@ selectExpr
 	;

 count
-	: ^(COUNT { Out("count("); }  ( distinctOrAll ) ? countExpr { Out(")"); } )
+	: ^(c=COUNT { OutAggregateFunctionName(c); Out("("); }  ( distinctOrAll ) ? countExpr { Out(")"); } )


Does this invalidate my previous comment? I mean, was this causing the SQL-Server HQL count to count_big mapping to be ignored by this piece of code, causing NHibernate to anyway use SQL-Server int32 count?

I mean, was this causing the SQL-Server HQL count to count_big mapping to be ignored by this piece of code, causing NHibernate to anyway use SQL-Server int32 count?

Correct, count was hardcoded, which was the reason why changing from count to count_big in CountBigQueryFunction didn't worked.

…_big

…_big (nhibernate#2061) Co-authored-by: Alexander Zaytsev <[email protected]>

hazzik · 2020-04-21T20:46:11Z

src/NHibernate/Dialect/Function/ISQLFunctionExtended.cs

+		/// <summary>
+		/// The function name or <see langword="null"/> when multiple functions/operators/statements are used.
+		/// </summary>
+		string FunctionName { get; }


I think this should be renamed to Name to reduce need of obsoleting things.

Why not, renamed in #2359.

hazzik · 2020-04-21T20:49:04Z

src/NHibernate/Hql/Ast/ANTLR/Tree/AggregateNode.cs

@@ -19,6 +20,19 @@ public AggregateNode(IToken token)
 		{
 		}

+		public string FunctionName


I think this could be an overgeneralization and should be refactored into a node-specific code unless you see that it can be extended for functions other than count and count_big

This may be extended for string concatenation, which is supported by some databases (Sql server, MySql, Postgresql and Oracle), where different function names are used (LISTAGG, GROUP_CONCAT, STRING_AGG).

@maca88 can you provide some sort of prototype of string_agg? I'm dying to use this with linq provider

To my knowledge it is not possible to implement it without modifying NHibernate source code. It is on my todo list for the next release, so unfortunately you will have to wait.

hazzik reviewed Oct 3, 2019

View reviewed changes

src/NHibernate/Linq/Visitors/HqlGeneratorExpressionVisitor.cs Outdated Show resolved Hide resolved

maca88 force-pushed the CountBig branch from 86f6cd0 to 89e9933 Compare February 21, 2020 21:59

maca88 changed the title ~~WIP - Reduce cast usage for COUNT aggregate and add support for Mssql count_big~~ Reduce cast usage for COUNT aggregate and add support for Mssql count_big Feb 21, 2020

maca88 mentioned this pull request Feb 22, 2020

Add multiple arguments support for ISQLFunction #2316

Merged

fredericDelaporte reviewed Mar 29, 2020

View reviewed changes

maca88 added 4 commits March 30, 2020 18:04

Reduce cast usage for COUNT aggregate and add support for Mssql count…

93cd848

…_big

Rename ISQLFunctionExtended interface to ISQLFunctionExtended

3a8dc8c

Fix CountBigQueryFunction ReturnType method

df065e1

Remove unneeded pragma warning

5f5ad60

maca88 force-pushed the CountBig branch from dca8472 to 5f5ad60 Compare March 30, 2020 16:20

fredericDelaporte previously approved these changes Mar 30, 2020

View reviewed changes

hazzik previously approved these changes Mar 30, 2020

View reviewed changes

Encapsulate implementation details

83715ee

hazzik dismissed stale reviews from fredericDelaporte and themself via 83715ee March 31, 2020 20:15

Code cleanup

bc4b6e5

hazzik approved these changes Mar 31, 2020

View reviewed changes

fredericDelaporte merged commit c0f5042 into nhibernate:master Apr 1, 2020

fredericDelaporte added c: Core c: DataProviders / Dialects p: Minor r: Fixed t: Improvement labels Apr 1, 2020

fredericDelaporte added this to the 5.3 milestone Apr 1, 2020

georgi-yakimov pushed a commit to georgi-yakimov/nhibernate-core that referenced this pull request Apr 16, 2020

Reduce cast usage for COUNT aggregate and add support for Mssql count…

2b73276

…_big (nhibernate#2061) Co-authored-by: Alexander Zaytsev <[email protected]>

hazzik reviewed Apr 21, 2020

View reviewed changes

maca88 mentioned this pull request Apr 22, 2020

Rename ISQLFunctionExtended.FunctionName to Name #2359

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce cast usage for COUNT aggregate and add support for Mssql count_big #2061

Reduce cast usage for COUNT aggregate and add support for Mssql count_big #2061

maca88 commented Mar 15, 2019 •

edited

Loading

maca88 commented Feb 21, 2020

fredericDelaporte Mar 29, 2020

maca88 Mar 29, 2020

fredericDelaporte Mar 29, 2020

maca88 Mar 29, 2020 •

edited

Loading

hazzik Apr 21, 2020

maca88 Apr 22, 2020

hazzik Apr 21, 2020

maca88 Apr 22, 2020

Nazgaul Aug 9, 2020

maca88 Aug 9, 2020

	object count = s.CreateQuery("select count(*) from Simple").UniqueResult();
	Assert.IsTrue(count is Int64);

Reduce cast usage for COUNT aggregate and add support for Mssql count_big #2061

Reduce cast usage for COUNT aggregate and add support for Mssql count_big #2061

Conversation

maca88 commented Mar 15, 2019 • edited Loading

maca88 commented Feb 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maca88 Mar 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maca88 commented Mar 15, 2019 •

edited

Loading

maca88 Mar 29, 2020 •

edited

Loading