Add query support for the static methods of System.Decimal #1533

weelink · 2018-01-15T15:04:50Z

Fixes #831

weelink · 2018-01-15T15:07:11Z

Since this is my first contribution and the change was too easy, could someone inspect this PR more than usual?

fredericDelaporte

This is just a quick first review. FireBird fails because Value is a keyword and must be escaped with back-tilt in the mappings, by specifying the Column property.

fredericDelaporte · 2018-01-15T18:22:29Z

src/NHibernate/Linq/Functions/DecimalGenerator.cs

+				ReflectHelper.GetMethodDefinition(() => decimal.Round(default(decimal))),
+				ReflectHelper.GetMethodDefinition(() => decimal.Round(default(decimal), default(int))),
+				ReflectHelper.GetMethodDefinition(() => decimal.Round(default(decimal), default(MidpointRounding))),
+				ReflectHelper.GetMethodDefinition(() => decimal.Round(default(decimal), default(int), default(MidpointRounding))),


Is MidpointRounding argument actually supported? It looks likely to me it is in fact ignored. In such case, better do as the Math generator do: do not support overloads with this argument.

Probably not. I'll remove this option. It worked in the tests because I was passing a Func<> and not an Expression<>.

hazzik · 2018-01-15T20:51:12Z

src/NHibernate.Test/NHSpecificTest/GH0831/FixtureByCode.cs

+			CanHandle(e => decimal.Subtract(2m, e.Value) > 1m);
+		}
+
+		private void CanHandle(Func<Entity, bool> predicate)


Shouldn't this be Expression<Func<Entity, bool>>?

I've updated the code so that it handles Expression<Func<Expression, bool>>

fredericDelaporte

Some database needs to be ignored for some tests, see comments.

SQLite completly fails. Maybe due to its broken support of decimal (it does not actually support them, it stores them as float instead ; TestDialect.HasBrokenDecimalType is true for SQLite due to this), but I would have still expected other test results for it. It needs some investigation I think.

fredericDelaporte · 2018-01-17T11:50:14Z

src/NHibernate.Test/NHSpecificTest/GH0831/FixtureByCode.cs

+		[Test]
+		public void CanHandleCeiling()
+		{
+			CanHandle(e => decimal.Ceiling(e.EntityValue) > 1.0m);


Oracle8iDialect bug showcased here. Its ceiling function is registered under the name ceil instead of being registered as ceiling.
RegisterFunction("ceil", new StandardSQLFunction("ceil"));
Changing it would be a breaking change, so instead an additional registration would be needed, by adding the line:

RegisterFunction("ceiling", new StandardSQLFunction("ceil"));

Since this bug is a bit unrelated to #831, I am adding a PR for it (#1535).

I noticed this as well and wanted to create a new bug for it, but you beat me to it

fredericDelaporte · 2018-01-17T12:59:09Z

src/NHibernate.Test/NHSpecificTest/GH0831/FixtureByCode.cs

+		[Test]
+		public void CanHandleRemainder()
+		{
+			CanHandle(e => decimal.Remainder(e.EntityValue, 2) == 0);


Some databases (SQLServerCE) do not support remainder on decimal. So a new property on TestDialect is needed, like SupportsModuloOnDecimal, true by default, and overridden to false in MsSqlCe40TestDialect.

Then remainder test should Assume.That(TestDialect.SupportsModuloOnDecimal, Is.True);

Firebird has a special semantic for modulo on non integer arguments: it rounds them beforehand. Thus the test failure. I think we should just consider it does not support modulo on decimal, so override SupportsModuloOnDecimal to false in FirebirdTestDialect too.

fredericDelaporte · 2018-01-17T13:02:20Z

src/NHibernate.Test/NHSpecificTest/GH0831/FixtureByCode.cs

+		[Test]
+		public void CanHandleDivide()
+		{
+			CanHandle(e => decimal.Divide(e.EntityValue, 1.25m) < 1);


Some databases (Firebird) do not support divide/multiply on decimal. So a new property on TestDialect is needed, like SupportsDecimalMultiplication, true by default, and overridden to false in FirebirdTestDialect.

Then divide and multiply tests should Assume.That(TestDialect.SupportsDecimalMultiplication, Is.True);

fredericDelaporte · 2018-01-17T13:07:01Z

src/NHibernate.Test/NHSpecificTest/GH0831/FixtureByCode.cs

+		[Test]
+		public void CanHandleAdd()
+		{
+			CanHandle(e => decimal.Add(e.EntityValue, 2) > 3.0m);


Its failure on Firebird seems to me due to some Firebird limitation. It looks like the operation on the column is enough to cause Firebird to lose track of the data type.
Maybe a workaround could be to add a cast, but I think we should just ignore that Firebird limitation with something like:

Assume.That(TestDialect.SupportsNonDataBoundCondition, Is.True);

Granted, here the condition is data bound, but the addition (or subtraction) on the data causes the same trouble. And I lack an idea for a new TestDialect property name for such a case.

weelink · 2018-01-17T14:43:07Z

I need to investigate the problems with SQLite a bit more. The results of executing the queries in the test are not consistent with what I see if I run the queries myself in a SQLite client. E.g. 'ceiling' is not supported if I run the query in the client, but I get no exception in the test. And the CanHandleAdd-test, that is doing a simple where (EntityValue + x) > y, returns no result.

There might be something strange going on.

fredericDelaporte · 2018-01-17T18:14:12Z

If some functions are not supported and have to be ignored, something like

Assume.That(Dialect.Functions, Does.ContainKey("ceiling"), Dialect + " doesn't support ceiling function.");

should take care of it. Of course this would also ignore the trouble for Oracle which only registers ceil. But since this is not the subject of this PR to fix registered functions, it is ok. (The SQL ANSI standard supports both ceil and ceiling. Many db implements both (MySql, Postgres, Firebird, ...), some implements only one (Oracle, SQL Server).)

Update: now better use AssumeFunctionSupported inherited from TestCase.

weelink · 2018-01-18T13:12:40Z

It seems that NHibernate and/or SQLite are in a disagreement about how to interpret the return type of the methods. The following does not work:

session.Query<Entity>()
    .Where(e => decimal.Add(e.EntityValue, 2) > 3.0m); // Results in an empty list, but several are expected

Adding an explicit Convert.ToDecimal does return the correct results:

session.Query<Entity>()
    .Where(e => Convert.ToDecimal(decimal.Add(e.EntityValue, 2)) > 3.0m) // Results in the expected list

This will be translated into cast(entity0_.EntityValue+@p0 as NUMERIC). This is not necessary if I execute the query manually in a client, so I don't think the problem is in SQLite.

@fredericDelaporte Do you have a possible direction for a solution? I'm not familiar with the codebase enough to figure it out on my own.

fredericDelaporte · 2018-01-18T14:26:40Z

This is not necessary if I execute the query manually in a client, so I don't think the problem is in SQLite.

Executing the query manually is probably done without using System.Data.SQLite, so the trouble could be there too.

Another cause may be a kind of parameter type mismatch with the resulting type from addition. So this may be dodged by using .MappedAs(NHibernateUtil.Double) or some other type on 3.0m. Or maybe by changing the decimal type registration in SQLiteDialect. It is currently numeric, but you could try with real which is the actual type used by SQLite. (Well, numeric can also result into integer type being used, if the value is indeed an integer.)
(The SQLite typing system looks quite messy to me. It has only five "in-memory" types (see data types, but can store them on disk with many more data-types, for which I am not finding an official documentation.)

By the way, have you tried the query with an explicitly decimal literal? decimal.Add(e.EntityValue, 2m)

fredericDelaporte · 2018-01-18T14:29:18Z

src/NHibernate.Test/NHSpecificTest/GH0831/FixtureByCode.cs

+			CanHandle(e => decimal.Subtract(2m, e.EntityValue) > 1m);
+		}
+
+		private void CanHandle(Expression<Func<Entity, bool>> predicate)


A more complete test would be to not only test filtering (what does this CanHandle, which could be renamed CanFilter) but also test selecting.

Can you add a CanSelect test ?

Good idea. Will add it shortly

fredericDelaporte · 2018-01-20T13:01:48Z

src/NHibernate.Test/NHSpecificTest/GH0831/FixtureByCode.cs

+			}
+		}
+
+		private void IgnoreIfNotSupported(string function)


With the merge of #1535 in master, this helper should be removed for using AssumeFunctionSupported (inherited from TestCase) instead.

weelink · 2018-01-21T11:37:02Z

I've added a CanSelect test.

It turns out that the return types are different. The return type of the mod function is Int32, but decimal.Remainder returns a decimal. The IQueryTranslator correctly returns a collection of type int, and then it crashes because it wants to add the (int) results to the decimal collection in NHibernate.Engine.Query.HQLQueryPlan line 149.

~~To make the test pass for SQL Server, I registered a new mod function to the dialects. But it is a temporary hack that I'll remove later when I figured out how deep the rabbit hole goes.~~

Adding an explicit cast to decimal solved this mismatch.

From what I understand from the documentation of SQLite, it doesn't have a type system like other databases do. It determines the type of the value by looking at the value, not the column definition. It ignores the declared type.

Changing the parameter to a decimal explicitly (i.e. 3m) doesn't help, but .MappedAs(NHibernateUtil.Double) solves the problem.

…thub.com/weelink/nhibernate-core into feature/GH0831_AddQuerySupportForDecimal

fredericDelaporte · 2018-01-21T18:29:48Z

It turns out that the return types are different. The return type of the mod function is Int32, but decimal.Remainder returns a decimal.

So here we have a behavior mismatch to investigate. In .Net runtime, executing decimal.Remainder(5.2m, 5.1m) yields 0.1m. So using a function yielding only int to emulate this cannot yield the same result.

Should we accept such a mismatch? Well, if no database implements a decimal remainder yielding non integer results, it would be moot to not accept it. SQL Server seems to yield only integer results by example.

fredericDelaporte · 2018-01-21T18:43:58Z

src/NHibernate.Test/NHSpecificTest/GH0831/FixtureByCode.cs

+			{
+				IEnumerable<decimal> inSession = session.Query<Entity>().Select(predicate).ToList();
+
+				Assert.That(inSession.OrderBy(x => x), Is.EqualTo(expected.OrderBy(x => x)).Within(0.001M));


I would rather avoid the order by on result values, which may hide a trouble (granted, unlikely). Instead, the inSession query could be completed by an .OrderBy(e => e.EntityValue) before the Select, which should ensure the db results have been computed on a list having the same ordering than the in-memory one.

fredericDelaporte · 2018-01-21T18:46:59Z

src/NHibernate/Linq/Functions/DecimalGenerator.cs

+					return treeBuilder.Multiply(expressions[0], expressions[1]);
+				case "remainder":
+					HqlMethodCall mod = treeBuilder.MethodCall("mod", expressions[0], expressions[1]);
+					return treeBuilder.Cast(mod, typeof(decimal));


Clearly a better solution. Now is it needed to have the cast in the resulting SQL query? If no, you could use a TransparentCast instead, which is just a hint for the HQL engine, and does not go into the generated SQL query. (Well, for SQLite, it does still perform the cast in SQL too, because that db has special needs.)

SQL Server CE doesn't support this Firebird rounds the values before calculating

Actually, there is possibly incorrect generation for compare. Need to add tests which consider nullable columns

weelink · 2018-03-03T12:41:32Z

Thanks for all the help

hazzik · 2018-03-03T13:39:56Z

@fredericDelaporte if my changes are OK. I'll merge the PR in a couple of hours.

This reverts commit 63a5b2a.

hazzik · 2018-03-03T23:22:08Z

I've reverted Decimal.Truncate because it needs to have more work.

weelink · 2018-03-04T10:01:08Z

I completely forgot about Decimal.Truncate. I'll try to make it work.

In Firebird the function is called 'trunc' MySQL requires the number of decimals to be specified

weelink · 2018-03-05T09:42:57Z

The SQL Server definition can be solved by using the VarArgsSQLFunction. Not very happy with this solution, but I'm not seeing a different possibility.
We could consider adding something that can handle these situations better. But that can be done in a separate PR.

hazzik · 2018-03-05T10:32:20Z

The SQL Server definition can be solved by using the VarArgsSQLFunction. Not very happy with this solution, but I'm not seeing a different possibility.

SQLFunctionTempate works better in that situation.

fredericDelaporte · 2018-03-05T19:15:44Z

src/NHibernate/Dialect/SybaseSQLAnywhere10Dialect.cs

@@ -343,6 +343,8 @@ protected virtual void RegisterMiscellaneousFunctions()
 			RegisterFunction("transactsql", new StandardSQLFunction("transactsql", NHibernateUtil.String));
 			RegisterFunction("varexists", new StandardSQLFunction("varexists", NHibernateUtil.Int32));
 			RegisterFunction("watcomsql", new StandardSQLFunction("watcomsql", NHibernateUtil.String));
+			RegisterFunction("truncnum", new StandardSafeSQLFunction("truncnum", 2));


I do not think that we should register specific function names for being available in HQL when using their specific dialect, since HQL should be database agnostic. RegisterFunction("truncnum" should not have been added.

I think opposite. Moreover, this is how it always was: dialect specific functions + aliases to the more common name.

"Always" widely depends on dialect. Some do register specific names for some of their functions in addition to the generic one, others do not. (By example SQL-Server registers only generic names for getdate, len, datepart, charindex, same for Postgres and now, ...)
It looks to me as something quite accidental.

This practice of registering specific names does not promote portability of HQL, while an ORM is usually meant to be database agnostic. That is why I think this is a bad thing, causing me to prefer not adding new specific names when we have a generic one. (Removing them after a release would of course be a breaking change for those using them.)

Add query support for the static methods of System.Decimal nhibernate…

0e6d7a9

…#831

fredericDelaporte reviewed Jan 15, 2018

View reviewed changes

hazzik reviewed Jan 15, 2018

View reviewed changes

hazzik and others added 4 commits January 16, 2018 09:55

Fix type of predicate to be expression

ef0490e

Add missing namespace import

10d0567

Merge branch 'master' into feature/GH0831_AddQuerySupportForDecimal

ec6ae5d

Apply mapping between Decimal.* and SQL functions

6b86b78

fredericDelaporte added the t: Fix label Jan 16, 2018

fredericDelaporte reviewed Jan 17, 2018

View reviewed changes

Ignore tests if the dialect does not support the function

d8190c5

fredericDelaporte reviewed Jan 18, 2018

View reviewed changes

fredericDelaporte reviewed Jan 20, 2018

View reviewed changes

weelink added 4 commits January 20, 2018 15:04

Merge branch 'master' into feature/GH0831_AddQuerySupportForDecimal

bda62c9

Replace IgnoreIfNotSupported with TestCase.AssumeFunctionSupported

1926597

Add mod-function that expects a decimal

8c98694

Merge branch 'master' into feature/GH0831_AddQuerySupportForDecimal

11bfbf2

weelink added 2 commits January 21, 2018 13:26

Replace 'mod_decimal'-hack with casting the result to decimal

f16a8ef

Merge branch 'feature/GH0831_AddQuerySupportForDecimal' of https://gi…

4724f23

…thub.com/weelink/nhibernate-core into feature/GH0831_AddQuerySupportForDecimal

fredericDelaporte reviewed Jan 21, 2018

View reviewed changes

hazzik and others added 4 commits January 22, 2018 22:28

Merge branch 'master' into feature/GH0831_AddQuerySupportForDecimal

56a9aae

Replace Cast with TransparentCast to avoid casting in the database

6359afb

Introduce a property to indicate if mod on decimal is supported

aba2f26

SQL Server CE doesn't support this Firebird rounds the values before calculating

Merge branch 'master' into feature/GH0831_AddQuerySupportForDecimal

f28a34e

hazzik previously approved these changes Mar 3, 2018

View reviewed changes

hazzik added 4 commits March 4, 2018 01:43

Merge DecimalCompareGenerator into CompareGenerator

16719fd

Merge DecimalFloorGenerator & DecimalCeilingGenerator into MathGenerator

28dde43

Adjust RoundGenerator to consider Math.Round methods

8906825

Implement Decimal.Truncate

63a5b2a

hazzik requested a review from fredericDelaporte March 3, 2018 13:10

Make generators internal

50f27c4

fredericDelaporte previously approved these changes Mar 3, 2018

View reviewed changes

Revert "Implement Decimal.Truncate"

a533078

This reverts commit 63a5b2a.

hazzik dismissed fredericDelaporte’s stale review via a533078 March 3, 2018 23:18

hazzik previously approved these changes Mar 3, 2018

View reviewed changes

weelink changed the title ~~Add query support for the static methods of System.Decimal~~ WIP Add query support for the static methods of System.Decimal Mar 4, 2018

Add support for Decimal.Truncate

7c1594a

In Firebird the function is called 'trunc' MySQL requires the number of decimals to be specified

weelink dismissed hazzik’s stale review via 7c1594a March 4, 2018 12:54

weelink changed the title ~~WIP Add query support for the static methods of System.Decimal~~ Add query support for the static methods of System.Decimal Mar 4, 2018

Redefine truncate function for MySQL and SQL Server

3abadf6

Fix truncate registration

f6c5038

hazzik approved these changes Mar 5, 2018

View reviewed changes

hazzik merged commit 56e542e into nhibernate:master Mar 5, 2018

hazzik added the r: Fixed label Mar 5, 2018

weelink deleted the feature/GH0831_AddQuerySupportForDecimal branch March 5, 2018 17:02

fredericDelaporte reviewed Mar 5, 2018

View reviewed changes

fredericDelaporte mentioned this pull request Mar 7, 2018

Add support for single-argument truncate to dialects that do not support it natively #1597

Merged

Add query support for the static methods of System.Decimal #1533

Add query support for the static methods of System.Decimal #1533

Uh oh!

Conversation

weelink commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weelink commented Jan 15, 2018

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weelink Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weelink commented Jan 17, 2018

Uh oh!

fredericDelaporte commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weelink commented Jan 18, 2018

Uh oh!

fredericDelaporte commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weelink commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weelink commented Mar 3, 2018

Uh oh!

hazzik commented Mar 3, 2018

Uh oh!

hazzik commented Mar 3, 2018

Uh oh!

weelink commented Mar 4, 2018

Uh oh!

weelink commented Mar 5, 2018

Uh oh!

hazzik commented Mar 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weelink commented Jan 15, 2018 •

edited

Loading

weelink Jan 16, 2018 •

edited

Loading

fredericDelaporte left a comment •

edited

Loading

fredericDelaporte Jan 17, 2018 •

edited

Loading

fredericDelaporte Jan 17, 2018 •

edited

Loading

fredericDelaporte commented Jan 17, 2018 •

edited

Loading

fredericDelaporte commented Jan 18, 2018 •

edited

Loading

weelink commented Jan 21, 2018 •

edited

Loading

fredericDelaporte commented Jan 21, 2018 •

edited

Loading