Reduce cast usage for aggregate functions #2036

maca88 · 2019-03-02T16:12:20Z

This PR uses the dialect specific SUM function to determine whether cast is needed or not and also takes nullable types into consideration, which fixes #2029.
Note:
The cast usage was not reduced for SQLite as it needs it when sum expression contains a parameter.

Edit: this PR has been extended to other aggregate functions.

src/NHibernate.Test/NHSpecificTest/GH2029/Fixture.cs

hazzik

Some of the logic is not necessary and/or, probably, needs to be extended to other functions of similar nature.

src/NHibernate/Linq/Visitors/HqlGeneratorExpressionVisitor.cs

maca88 · 2019-03-11T19:27:13Z

I've made a more sophisticated logic that can be used also for other aggregates like COUNT and AVG. Instead of checking the ReturnedClass property, the logic is now comparing SqlTypes which should be more correct as the cast is done on the database level. The logic was applied also for AVG but not for COUNT as I found out that the registered function count_big for MsSql2000Dialect is not used for hql queries and would not add a cast for a long type (COUNT in Sql Server returns an int). It seems that NH-865 was never tested. I will try to address the count issue for Sql Server in a separate PR.

maca88 · 2019-03-11T19:30:55Z

src/NHibernate/Linq/Visitors/HqlGeneratorExpressionVisitor.cs

+				: persister.EntityMetamodel.PropertyTypes[index.Value];
+		}
+
+		private string TryGetEntityName(MemberExpression memberExpression)


Copied from #1996.

After doing #2079 I found out that this method does not cover all cases. I will update this method and try to add additional tests that cover the edge cases.

This method was improved and exposed for internal usage by #1996. I am thinking to leave this PR as WIP until #1996 get merged in order to replace this method with the new one.

PR updated

maca88 · 2019-04-07T19:06:36Z

src/NHibernate/Linq/Visitors/HqlGeneratorExpressionVisitor.cs

+						? _hqlTreeBuilder.Cast(VisitExpression(expression.Operand).AsExpression(), expression.Type)
+						// Make a transparent cast when an IType exists, so that it can be used to retrieve the value from the data reader
+						: existType
+							? _hqlTreeBuilder.TransparentCast(VisitExpression(expression.Operand).AsExpression(), expression.Type)


I have to recheck this as TransparentCast does support only a few types.

Added an additional check for calling TransparentCast , as there is an edge case when casting to a custom registered type.

maca88 · 2019-04-07T19:07:33Z

Set to WIP as I need to revisit this PR, see the comments above.

maca88 · 2019-10-01T17:28:38Z

Rebased and updated TryGetEntityName (from #1996) and SupportsType (from #2079) methods.

maca88 · 2019-10-11T18:16:24Z

src/NHibernate/Util/ExpressionsHelper.cs

@@ -225,10 +213,24 @@ public static class ExpressionsHelper
 							out currentComponentType);
 						break;
 					case IAbstractComponentType componentType:
+						// Concatenate the component property path in order to be able to use EntityMetamodel.GetPropertyType to retrieve the type.
+						// As GetPropertyType supports only components, do not concatenate when dealing with collection composite elements or elements.
+						if (!currentType.IsAnyType)


Thanks for cleaning the code. I've removed this condition as it is no longer needed because AnyType also implements IAssociationType, so it will be handled by the above case.

hazzik · 2019-10-11T22:52:01Z

src/NHibernate/Util/ExpressionsHelper.cs

+			if (memberPaths.Count == 0)
+			{
+				memberPath = currentEntityPersister != null || currentComponentType != null ? member.Path : null;
+				mappedType = GetType(currentEntityPersister, currentType, member, sessionFactory, out _);


Are you using this GetType method in other PRs? If no, I think the last argument needs to be removed.

No, I've removed it.

maca88 · 2019-10-29T20:07:53Z

I've updated the TryGetMappedType method to support polymorphic queries, coalesce and conditional expressions, which will be needed for #2079 in order to detect parameter types.

maca88 · 2019-11-12T21:27:02Z

src/NHibernate/Dialect/Function/CastFunction.cs

 				sqlType = factory.Dialect.GetCastTypeName(sqlTypeCodes[0]);
-				if (sqlType == null)
-				{
-					//TODO: never reached, since GetTypeName() actually throws an exception!


It doesn't make sense using the type name when the dialect doesn't support the cast. If the cast would be supported, GetCastTypeName should return it.

…ions

maca88 · 2019-11-13T21:38:19Z

Rebased.

fredericDelaporte

I have adjusted the TryGetMappedTests.Assert... names for them to be more explicit.
And I have adjusted this PR title.

fredericDelaporte mentioned this pull request Mar 5, 2019

Incorrect SQL when comparing a guid and string column in Sql Server #2040

Closed

fredericDelaporte reviewed Mar 10, 2019

View reviewed changes

src/NHibernate.Test/NHSpecificTest/GH2029/Fixture.cs Outdated Show resolved Hide resolved

src/NHibernate.Test/NHSpecificTest/GH2029/Fixture.cs Outdated Show resolved Hide resolved

fredericDelaporte added this to the 5.3 milestone Mar 10, 2019

fredericDelaporte added t: Improvement c: Linq Provider p: Minor labels Mar 10, 2019

fredericDelaporte previously approved these changes Mar 10, 2019

View reviewed changes

hazzik previously requested changes Mar 10, 2019

View reviewed changes

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

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

maca88 dismissed fredericDelaporte’s stale review via 171dd10 March 11, 2019 19:25

maca88 commented Mar 11, 2019

View reviewed changes

hazzik self-requested a review March 13, 2019 01:22

maca88 changed the title ~~Reduce cast usage for SUM aggregate function~~ WIP - Reduce cast usage for SUM aggregate function Mar 14, 2019

maca88 mentioned this pull request Mar 15, 2019

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

Merged

maca88 changed the title ~~WIP - Reduce cast usage for SUM aggregate function~~ Reduce cast usage for SUM aggregate function Mar 16, 2019

fredericDelaporte previously approved these changes Apr 7, 2019

View reviewed changes

maca88 commented Apr 7, 2019

View reviewed changes

maca88 changed the title ~~Reduce cast usage for SUM aggregate function~~ WIP - Reduce cast usage for SUM aggregate function Apr 7, 2019

maca88 dismissed fredericDelaporte’s stale review via 7b61bae April 10, 2019 19:37

maca88 force-pushed the ReduceSumCast branch from 7b61bae to e505147 Compare October 1, 2019 17:21

maca88 changed the title ~~WIP - Reduce cast usage for SUM aggregate function~~ Reduce cast usage for SUM aggregate function Oct 1, 2019

maca88 mentioned this pull request Oct 8, 2019

Enhance nullability check for "==" and "!=" operators for LINQ provider #1996

Merged

maca88 commented Oct 11, 2019

View reviewed changes

hazzik reviewed Oct 11, 2019

View reviewed changes

maca88 mentioned this pull request Nov 12, 2019

WIP - Improve select clause transformation for Linq provider #2079

Open

maca88 commented Nov 12, 2019

View reviewed changes

maca88 and others added 12 commits November 13, 2019 18:48

Replace TryGetEntityName with TryGetMappedType

b4027c6

Merge two conditions to reduce complexity

7ee50d7

Split TryGetMappedType into two methods

ae7a80c

Avoid while(true)

256f537

Move getting next member into swith-case blocks

418a588

Inline ProcessComponentType

d277689

Fix typos

a49ddef

Small corrections

68f5407

Remove unneeded GetType parameter

315b573

Fix typo

b691d1d

Add support for polymorphic queries, coalesce and conditional express…

9ea2d63

…ions

Revert cast function change

17422d7

maca88 force-pushed the ReduceSumCast branch from 2e2f42f to 17422d7 Compare November 13, 2019 21:20

Adjust test namings

660f15d

fredericDelaporte changed the title ~~Reduce cast usage for SUM aggregate function~~ Reduce cast usage for aggregate functions Feb 16, 2020

Merge branch 'master' into ReduceSumCast

392fb1e

fredericDelaporte approved these changes Feb 16, 2020

View reviewed changes

maca88 mentioned this pull request Feb 17, 2020

LINQ "==" operator generates OR with IS NULL #1860

Closed

fredericDelaporte merged commit d4206e2 into nhibernate:master Feb 20, 2020

fredericDelaporte added the r: Fixed label Feb 20, 2020

This was referenced Jul 20, 2020

Fix invalid cast on nullable custom type with Linq #2438

Merged

Fix detecting parameter types in subqueries for Linq provider #2443

Merged

Fix casting unsigned numeric types for Linq provider #2447

Merged

gliljas mentioned this pull request Aug 26, 2020

Cast operation fails when an enum is mapped as an AnsiString #2499

Merged

maca88 mentioned this pull request Jan 24, 2021

Fix enum detection when using OfType for Linq provider #2651

Merged

maca88 mentioned this pull request Feb 7, 2021

Fix wrong column selection when using OfType in a Linq query #2669

Merged

This was referenced Jul 27, 2021

Fix property type retrieval for Composite User Type in LINQ #2877

Merged

Fix casting to unmapped interface in LINQ #2879

Merged

bahusoid mentioned this pull request Feb 21, 2022

Enable test accessing Component's Parent property in LINQ #3024

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce cast usage for aggregate functions #2036

Reduce cast usage for aggregate functions #2036

maca88 commented Mar 2, 2019 •

edited by fredericDelaporte

Loading

hazzik left a comment

maca88 commented Mar 11, 2019 •

edited

Loading

maca88 Mar 11, 2019

maca88 Apr 7, 2019

maca88 Apr 10, 2019

maca88 Apr 7, 2019

maca88 Apr 10, 2019

maca88 commented Apr 7, 2019

maca88 commented Oct 1, 2019

maca88 Oct 11, 2019

hazzik Oct 11, 2019 •

edited

Loading

maca88 Oct 13, 2019

maca88 commented Oct 29, 2019

maca88 Nov 12, 2019

maca88 commented Nov 13, 2019

fredericDelaporte left a comment

Reduce cast usage for aggregate functions #2036

Reduce cast usage for aggregate functions #2036

Conversation

maca88 commented Mar 2, 2019 • edited by fredericDelaporte Loading

hazzik left a comment

Choose a reason for hiding this comment

maca88 commented Mar 11, 2019 • 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

maca88 commented Apr 7, 2019

maca88 commented Oct 1, 2019

Choose a reason for hiding this comment

hazzik Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maca88 commented Oct 29, 2019

Choose a reason for hiding this comment

maca88 commented Nov 13, 2019

fredericDelaporte left a comment

Choose a reason for hiding this comment

maca88 commented Mar 2, 2019 •

edited by fredericDelaporte

Loading

maca88 commented Mar 11, 2019 •

edited

Loading

hazzik Oct 11, 2019 •

edited

Loading