SQLite: Enable Decimal

[bricelam]

Similar to PR #19617, we can enable more decimal operations by leveraging UDFs. Unlike TimeSpan, however, there isn't an acceptable type we can convert to to perform the operations, so it will require roughly one UDF per operation. For example:

Done	.NET	SQL
✔	m1 + m2	ef_add($m1, $m2)
✔	m1 / m2	ef_divide($m1, $m2)
✔	m1 > m2	ef_compare($m1, $m2) > 0
✔	m1 >= m2	ef_compare($m1, $m2) >= 0
✔	m1 < m2	ef_compare($m1, $m2) < 0
✔	m1 <= m2	ef_compare($m1, $m2) <= 0
✔	m1 % m2	ef_mod($m1, $m2)
✔	m1 * m2	ef_multiply($m1, $m2)
✔	m1 - m2	ef_add($m1, ef_negate($m2))
✔	-m	ef_negate($m)
✔	Average(t => t.Decimal)	ef_avg(t.Decimal)
	Max(t => t.Decimal)	ef_max(t.Decimal)
	Min(t => t.Decimal)	ef_min(t.Decimal)
✔	Sum(t => t.Decimal)	ef_sum(t.Decimal)
	OrderBy(t => t.Decimal)	ORDER BY t.Decimal COLLATE EF_DECIMAL
	OrderByDescending(t => t.Decimal)	ORDER BY t.Decimal COLLATE EF_DECIMAL DESC

[bricelam]

I don’t think there’s much we can do for OrderBy...

[lokalmatador]

Hi, first time here looking to contribute to OSS projects. Being tagged as a good first issue, maybe I can give it a shot?

[bricelam]

@lokalmatador Sure! You might want to start small on this one (e.g. just do ef_compare and it’s operators) in your first PR.

[lokalmatador]

Great! I guess I start out by looking at your changes in the above mentioned PR (PR#19617) to get an idea on how to possibly achieve this. Also, I may ask one or another question the next days :)

[bricelam]

Feel free to start a draft PR to ask questions about the implementation

[bricelam]

(Added modulo as part of PR #20024)

[lokalmatador]

First of, thanks for helping me in getting my first contribution done. I hope I wasn't too much of a burden. Would it be OK to continue on this task? I'd be happy to.

[bricelam]

No burden at all! Please feel free to continue

[lokalmatador]

Well, then I'd continue with ef_{add, divide, multiply, subtract} and ef_negate.

[lokalmatador]

Draft PR is here

[bricelam]

So, @JonPSmith got me thinking again about ORDER BY and I think we could actually do it via collation.

connection.CreateCollation(
    "EF_DECIMAL",
    (x, y) => decimal.Compare(decimal.Parse(x), decimal.Parse(y));

SELECT *
FROM MyTable
ORDER BY DecimalColumn COLLATE EF_DECIMAL

[JonPSmith]

Wow, I wouldn't have thought of that!

I did DM @ErikEJ about this and it put me on to #18593 and I saw the Sqlite limitations page.

I use Sqlite in-memory as much as possible for unit testing. That's because a lot of people use 'standard' EF Core, i.e. don't use any of the SQL-specific parts, which means Sqlite in-memory works really well. I used Sqlite in-memory in my last client's project and they had an Azure CI/CD pipeline that includes unit tests and Sqlite in-memory works great!

So, of course, I am interested in differences between Sqlite and say SQL Server (a typical database people use). Other than string case sensitivity and filter/order on decimal, then Sqlite in-memory is a good fit for unit testing. I suspect I will go with a ValueConvertion to double if the database provider is Sqlite to overcome the decimal limitation.

PS. If you guys have any clever way to manage a real database in a CI/CD pipeline then I would love to know. Clearly I could call EnsureDeleted and then EnsureCreated in every unit test, but that is so slow!

[ajcvickers]

@JonPSmith In my experience, it's only the creation and dropping of databases that is really slow. (Also, dropping a database destabilizes SQL Server, so we avoid ever doing it when running tests on the C.I.) We have a database cleaner that uses our reverse engineering code to clear the database instead of dropping and re-creating it. We haven't productized it, but we're considering it. Also, it looks like @jbogard's Respawn does something similar.

[JonPSmith]

Hi @ajcvickers,

Yes, I have a cleaner too but I think yours might be better, but the real problem is migrations, i.e. the Model changes during development, which is going to happen say 2% (??) of the push to CI/CD pipeline.

I have a fix for handling changes to the Model that works well locally (and used it in real apps), i.e.

1. Create unique database names for unit tests, but with a given prefix.
2. If the Model changes the developer has to manually run a method that deletes all the databases with names with that prefix.

That works locally, but not for CI/CD. Here are two ideas I am thinking about.

1. At the start of each CI/CD I use my method to delete all the unit test databases before running any unit tests in the pipeline. That simple(ish), but it will be slow for the 98% times there is no Model change. Maybe speed in CI/CD isn't so important(?)
2. Add a test ahead of the 'delete all unit test databases' that checks if migrations are needed to a 'canary database'. If a migration was applied then it delete all the unit test databases. Quicker in 98%, but feels fragile.

Sorry, I'm unloading my ideas but I want to come up with something for an "article". If its off-topic then please ignore.

[ajcvickers]

@JonPSmith The key thing about the cleaner is that it can't be based on the EF model otherwise, as you point out, when the model changes it may no longer match the database structure. That's why the EF one uses the reverse engineering code to figure out what is in the database and then drop it all. The process used in the EF C.I. is that every test database is created if it doesn't already exist, and then cleaned. The database is then never dropped.

[JonPSmith]

@ajcvickers Wow - that is so cool! I want it :smile:

Obviously I hadn't looked at your cleaner properly before, but I have now. Its pretty complex so I want to reflect back what I think your cleaner does so you can correct me. I think it does the following:

1. If there is an existing database it
   1. Uses the reverse engineering code to read the schema
   2. The sql code in the cleaner then deletes all the indexes, foreign key (constraints?), tables, and sequences
   3. Then it runs some code given by the caller - for SQL Server this has code to delete other things like views, functions etc.
2. Then it creates the tables etc. defined by the current Model. I presume its uses the code in context.Database.EnsureCreated

Is that correct? If so that solves the C.I. problem, but actually makes the local unit testing much better too (no need to run a method when the Model changes). And I tried a few of your unit tests and the initial test is pretty quick too.

The next question is, can the SqlServerDatabaseCleaner be made available easily? The RelationalDatabaseCleaner doesn't access anything internal, but SqlServerDatabaseCleaner does.

Any comments?

[jbogard]

This is exactly what my Respawn tool does, ha! https://github.com/jbogard/Respawn

On Thu, Apr 9, 2020 at 5:38 AM Jon P Smith notifications@github.com wrote:

@ajcvickers https://github.com/ajcvickers Wow - that is so cool! I want it 😄

Obviously I hadn't looked at your cleaner properly before, but I have now. Its pretty complex so I want to reflect back what I think your cleaner does so you can correct me. I think it does the following:

1. If there is an existing database it
   1. Uses the reverse engineering code to read the schema
   2. The sql code in the cleaner then deletes all the indexes, foreign key (constraints?), tables, and sequences
   3. Then it runs some code given by the caller - for SQL Server this has code to delete other things like views, functions etc.
2. Then it creates the tables etc. defined by the current Model. I presume its uses the code in context.Database.EnsureCreated

Is that correct? If so that solves the C.I. problem, but actually makes the local unit testing much better too (no need to run a method when the Model changes). And I tried a few of your unit tests and the initial test is pretty quick too.

The next question is, can the SqlServerDatabaseCleaner be made available easily? The RelationalDatabaseCleaner doesn't access anything internal, but SqlServerDatabaseCleaner does.

Any comments?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/efcore/issues/19635#issuecomment-611458660, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZQMW5R5UOHMQOZFEHPILRLWQR5ANCNFSM4KIVZ3HA .

[JonPSmith]

Hi @jbogard,

I did look at Respawn. The EF Core SQL Server cleaner also deletes views, functions, procs, aggregates, types, schema, but I could copy the EF Core code for that. I notice Respawn is async, which means it can't be used in the unit test constructor, which is a pity. I have a look/think about this.

[jbogard]

No for xUnit you need to use its async initialization stuff.

On Thu, Apr 9, 2020 at 8:33 AM Jon P Smith notifications@github.com wrote:

Hi @jbogard https://github.com/jbogard,

I did look at Respawn. The EF Core SQL Server cleaner also deletes views, functions, procs, aggregates, types, schema, but I could copy the EF Core code for that. I notice Respawn is async, which means it can't be used in the unit test constructor, which is a pity. I have a look/think about this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/efcore/issues/19635#issuecomment-611530272, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZQMQV25XNH4VHORNBU4LRLXFB5ANCNFSM4KIVZ3HA .

[JonPSmith]

OK, looking at that now

[JonPSmith]

Hi @jbogard,

I made sure I looked at Respawn and it does a great job of removing all the data, but the EF Core cleaner deletes the actual tables in the database, i.e. Drop(MyTable). Dropping all the database tables, indexes, foreign keys , etc. is what I want, so that unit tests can handle changes in the EF Core Model when running unit tests.

Also @ajcvickers, looking at Respawn made me think - does your RelationalDatabaseCleaner handle circular table references? I assume that deleting the foreign keys first makes that work - correct?

[jbogard]

Right, we don’t want that for our integration tests. That’s taken care of via migrations.

On Fri, Apr 10, 2020 at 9:01 AM Jon P Smith notifications@github.com wrote:

Hi @jbogard https://github.com/jbogard,

I made sure I looked at Respawn and it does a great job of removing all the data, but the EF Core cleaner deletes the actual tables in the database, i.e. Drop(MyTable). Dropping all the database tables, indexes, foreign keys , etc. the what I want, so that unit tests can handle changes in the EF Core Model when running unit tests.

Also @ajcvickers https://github.com/ajcvickers, looking at Respawn made me think - does your RelationalDatabaseCleaner handle circular table references? I assume that deleting the foreign keys first makes that work - correct?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/efcore/issues/19635#issuecomment-612041023, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZQMR4HKOXJ74NUIK7T7TRL4RCDANCNFSM4KIVZ3HA .

[ajcvickers]

Catching up on this. We generally don't use migrations for our functional tests--remember our tests are for testing EF, not an application that uses EF, and hence we have models that change rapidly as we add new features/regression cases to the tests. So what we want is for the database to match the current model, which may be different from any previous models, and hence we re-create the database structure each time. So yes, @JonPSmith, your description seems correct to me.

Dropping database objects is something that has to be done in a specific order to avoid violating constraints--for example, dropping FK constraints before attempting to delete tables that reference the constraint.

The cleaner was written to handle all of the cases that the EF tests generate. It was not generalized beyond this. This means it hasn't been tested against any arbitrary database structure. This is why we haven't productized it--we would need to have confidence (tests) that it would work for at least a large percentage of customer databases. We're considering doing this, or finding some other way of making it usable by others.

[JonPSmith]

Thanks @ajcvickers for the update. You said

... hence we have models that change rapidly as we add new features/regression cases to the tests.

This is exactly the situation I, and others, find when running unit tests on an evolving/agile application. In particular I always run unit tests before I build a migration, so I have to handle Model changes before there is a migration. This means your cleaner design is just what I was looking for unit testing, both locally and in a C.I. pipeline.

I also understand making your current cleaner both work in all states and easy for people to use adds another item to your very full list of changes. Therefore I will try building my own cleaner, maybe only for SQL Server, and put it into my EfCore.TestSupport library. I haven't got time straight straight away, but hopefully I can do something before/when EF Core 5 comes out.

[ajcvickers]

@JonPSmith Sounds good. Let us know how it goes.

[lokalmatador]

Basic arithmetics (#20212) (add, sub, multiply, divide, negate) are done. I would just continue working down the list. I assume for the remaining operations t denotes an IEnumerable? Is there anything else to be aware of?

[bricelam]

Yes, t is an IEnumerable<TEntity>. I don't have any advice, but @smitpatel might. Aggregates over decimal columns are currently blocked in SqliteSqlTranslatingExpressionVisitor. OrderBy is blocked in SqliteQueryableMethodTranslatingExpressionVisitor.

[smitpatel]

For aggregate, register the functions an in SqliteSqlTranslatingExpressionVisitor generate translation to that custom SqlFunction.

For order by, now that we have CollateExpression, you can just wrapp the ordering expression in CollateExpression with EF_DECIMAL collation (for both TranslateOrderBy/TranslateThenBy).

[lokalmatador]

Thanks for the tips, will give it a shot during this rainy and snowy weekend!

[JonPSmith]

Hi @ajcvickers,

I have been using a version of the EnsureClean found in the EF Core test code for some time now with excellent results. I'm about to embark on writing the last chapter (hurrah!) of my book which is about unit testing and plan to update my EfCore.TestSupport open-source library to EF Core 5 and include an EnsureClean method which is based on a single class adapted from your Microsoft.EntityFrameworkCore.TestUtilities.RelationalDatabaseCleaner class.

My question is what do I need to do to recognize that this code came from the EF Core team? I have currently kept your header with your copyright and added a third line saying I have altered it. Is that enough?

// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
// Altered by Jon P Smith, GitHub @JonPSmith, https://www.thereformedprogrammer.net/

Here is a link to the altered class.

PS. My library is under an MIT licence.

[ajcvickers]

@JonPSmith I'm not a lawyer. :-) @Pilchie?

[terrajobst]

IANAL, but MIT requires attribution. We generally handle this by delivering a THIRD-PARTY-NOTICES.txt what we deliver with the apps/packages. See this one as an example.

[JonPSmith]

Thanks @terrajobst, I have updated the two classes with the following header

// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0.
// Available at https://github.com/dotnet/efcore/blob/main/LICENSE.txt
// Altered by Jon P Smith, GitHub @JonPSmith, https://www.thereformedprogrammer.net/

I have also added a section to my project's LICENSE file as follows

NOTE

Two classes in this library come from Microsoft's Entity Framework Core
and are therefore Copyright (c) .NET Foundation. All rights reserved.
And licensed under the Apache License, Version 2.0.
The licence is available at https://github.com/dotnet/efcore/blob/main/LICENSE.txt

I think that covers it, but if anyone wants me to do anything else I'm happy to oblige.

[ankurparekh1988]

Hello, I am looking for my first OSS contribution. Would like to know if this is still open and Can I start looking into this or if you can redirect me on one of open good first issue which has kind of priority ?

[bricelam]

@ankurparekh1988 Sure! See my and Smit's comments above for hints on where to start digging into the code. Note, just the aggregate functions are left to do.

[ankurparekh1988]

@bricelam @smitpatel I did try to understand the code and look into Smit's comment as well. However I am bit lost there. Can you please guide me on starting point and I can start from there.

[dresswithpockets]

@ankurparekh1988 are you still picking this up? if not, I've already done a bit of work to add aggregate operations and would be able to make a PR soon.

[ankurparekh1988]

@dresswithpockets I was working on it but than I got something urgent so I couldn't finish. Since you have already done the changes and can make PR. I would say go ahead. I will look into another open issue. Thanks.

[dresswithpockets]

cool.

@bricelam ive got a branch that adds support for the aggregate operations over decimal - how does back porting work? i cant find documentation for how y'all handle back porting changes, and this is a change i think would be appropriate to include in 6.x.

[bricelam]

We don't backport enhancements. Only very high impact bugs and security vulnerabilities. Send a PR against the main branch.

[bricelam]

But, if you want to release something for 6.0, you could create your own library that adds the missing translations. (Something similar to the EFCore.SqlServer.HierarchyId community project.)

[dresswithpockets]

Ah, understandable. I suppose I will have to do that, since we use 6.0. I'll send a PR against main soon, thanks!

[suatsuphi]

Sum(t => t.Decimal) ef_sum(t.Decimal)

hi, can you give an example? how is it ?

I found these example and TimeSpan working for me

            connection.CreateCollation( "ef_decimal", (x, y) => decimal.Compare(decimal.Parse(x), decimal.Parse(y)));
            connection.CreateFunction("ef_days", (TimeSpan value) => value.TotalDays);
            connection.CreateFunction("ef_timespan", (double value) => TimeSpan.FromDays(value));

[bricelam]

It should look something like this:

connection.CreateAggregate(
    "ef_sum",
    seed: null,
    (decimal? sum, decimal? value)
        => !value.HasValue
            ? sum
            : !sum.HasValue
                ? value
                : sum + value,
    isDeterministic: true);

Careful though, you might run into issue #26070.

[suatsuphi]

Hi, @bricelam thank you.

[flimtix]

@bricelam @suatsuphi Thank you for catching this bug. 👍 We use SQLite for unit testing and found that all queries containing a decimal do not work. This means that we cannot test the correctness of our queries.

Is there a schedule when this bug will be fixed? Or is there already a workaround?

[jvakos]

Any news on this. It's almost 4 years. At least give us a schedule or a workaround.

[roji]

@jvakos there's a workaround above in https://github.com/dotnet/efcore/issues/19635#issuecomment-1117563394. Other than that, it has indeed been 4 years, but during which this issue got only 12 votes, meaning that not a lot of users need it.

[njkoopman]

@roji I think it is still very relevant feature. We manly because we use SQLite for unit-testing and until now we are not able to unit-test any functions that sum, max, min or avg decimals in the database. The reason we use decimals and not doubles or singles is that decimals are precise.

The proposed workaround of @bricelam (quoted below) isn't working either, because an error is thrown when by SqliteQueryableAggregateMethodTranslator calling Sum with a decimal: NotSupportedException : SQLite cannot apply aggregate operator 'Sum' on expressions of type 'decimal'. Convert the values to a supported type, or use LINQ to Objects to aggregate the results on the client side.

//Stack trace:
SqliteQueryableAggregateMethodTranslator.Translate(MethodInfo method, EnumerableExpression source, IReadOnlyList`1 arguments, IDiagnosticsLogger`1 logger)
<>c__DisplayClass7_0.<Translate>b__0(IAggregateMethodCallTranslator t)
SelectEnumerableIterator`2.MoveNext()
Enumerable.TryGetFirst[TSource](IEnumerable`1 source, Func`2 predicate, Boolean& found)
Enumerable.FirstOrDefault[TSource](IEnumerable`1 source, Func`2 predicate)
RelationalAggregateMethodCallTranslatorProvider.Translate(IModel model, MethodInfo method, EnumerableExpression source, IReadOnlyList`1 arguments, IDiagnosticsLogger`1 logger)
RelationalSqlTranslatingExpressionVisitor.TranslateAggregateMethod(EnumerableExpression enumerableExpression, MethodInfo method, List`1 scalarArguments)
RelationalSqlTranslatingExpressionVisitor.TryTranslateAggregateMethodCall(MethodCallExpression methodCallExpression, SqlExpression& translation)
RelationalSqlTranslatingExpressionVisitor.VisitMethodCall(MethodCallExpression methodCallExpression)
MethodCallExpression.Accept(ExpressionVisitor visitor)
<8 more frames...>
ExpressionVisitor.Visit(Expression node)
QueryCompilationContext.CreateQueryExecutor[TResult](Expression query)
Database.CompileQuery[TResult](Expression query, Boolean async)
QueryCompiler.CompileQueryCore[TResult](IDatabase database, Expression query, IModel model, Boolean async)
<>c__DisplayClass9_0`1.<Execute>b__0()
CompiledQueryCache.GetOrAddQuery[TResult](Object cacheKey, Func`1 compiler)
QueryCompiler.Execute[TResult](Expression query)
EntityQueryProvider.Execute[TResult](Expression expression)
Queryable.Sum(IQueryable`1 source)

connection.CreateAggregate(
    "ef_sum",
    seed: null,
    (decimal? sum, decimal? value)
        => !value.HasValue
            ? sum
            : !sum.HasValue
                ? value
                : sum + value,
    isDeterministic: true);

Careful though, you might run into issue #26070.

[roji]

We manly because we use SQLite for unit-testing and until now we are not able to unit-test any functions that sum, max, min or avg decimals in the database.

It's generally discouraged to use a different database for testing (e.g. swap SQL Server for SQLite), since databases - and their providers - have different capabilities; this is an example of such a difference. See our testing docs for more information on this.