Basic arithmetics #20212

lokalmatador · 2020-03-07T15:35:06Z

No description provided.

lokalmatador · 2020-03-07T15:36:55Z

@bricelam draft for basic arithmetics and ef_negate

bricelam · 2020-03-13T23:35:25Z

src/EFCore.Sqlite.Core/Query/Internal/SqliteSqlTranslatingExpressionVisitor.cs

-                {
-                    typeof(decimal),
-                    typeof(TimeSpan),
-                    typeof(ulong)


We should file an issue to do the ulong ones too.

I guess it would make sense, yes. I'd be happy to work on this, when decimals are finished, if OK.

bricelam · 2020-03-13T23:42:44Z

src/EFCore.Sqlite.Core/Storage/Internal/SqliteRelationalConnection.cs

+                    name: "ef_add",
+                    (decimal? left, decimal? right) => left.HasValue && right.HasValue
+                        ? decimal.Add(left.Value, right.Value)
+                        : default(decimal?),


Can this just be...

(decimal? left, decimal? right) => left / right,

You mean left + right? Yes, it can - and is now.

bricelam · 2020-03-13T23:45:01Z

src/EFCore.Sqlite.Core/Storage/Internal/SqliteRelationalConnection.cs

+                            return decimal.Divide(dividend.Value, divisor.Value);
+                        }
+
+                        throw new DivideByZeroException();


Won't Divide just throw this?
Can all of this just be...

(decimal? dividend, decimal? divisor) => dividend / divisor,

Hm, at least the tests passed this way. But it's anyway not needed here, as error checking happens down the path inside Decimal.Dec.Calc.cs which I (see below comment) unfortunately did not realize earlier.

Fixed it.

bricelam · 2020-03-13T23:45:34Z

src/EFCore.Sqlite.Core/Storage/Internal/SqliteRelationalConnection.cs

+                    name: "ef_multiply",
+                    (decimal? left, decimal? right) => left.HasValue && right.HasValue
+                        ? decimal.Multiply(left.Value, right.Value)
+                        : default(decimal?),


Ditto. left * right

bricelam · 2020-03-13T23:46:07Z

src/EFCore.Sqlite.Core/Storage/Internal/SqliteRelationalConnection.cs

+                    name: "ef_negate",
+                    (decimal? m) => m.HasValue
+                        ? decimal.Negate(m.Value)
+                        : default(decimal?),


Ditto. -m

lokalmatador · 2020-03-14T05:56:54Z

Didn't realize there were also operators...for whatever reason. Need to check more carefully next time. Thanks for the hints.

lokalmatador · 2020-03-21T12:52:32Z

@bricelam sorry for the delay, been a little chaotic here recently. So I gave the error messages related to the above failures a look, but so far lack any idea how to approach this. Apparently, the failing test Microsoft.EntityFrameworkCore.ConfigurationPatternsTest.Can_register_multiple_context_types) also does not run locally and only gives this reason: LocalDb is not accessible on this platform. An external SQL Server must be configured. Any ideas how I could approach this? Thanks!

smitpatel · 2020-03-21T16:28:11Z

@lokalmatador - That test has intermittent failure due to high load on SqlServer. I recently disabled that test from running. Can you rebase on latest master? The error should go away. It is not an issue in your PR.

lokalmatador · 2020-03-21T17:48:22Z

@smitpatel Oh yeah, I can see it now. Thanks for the info.

lokalmatador · 2020-04-19T09:04:18Z

Is there anything open on this PR?

smitpatel

We need functional tests which performs decimal operations in query and assert generated SQL with it.

smitpatel · 2020-04-20T18:03:27Z

src/EFCore.Sqlite.Core/Query/Internal/SqliteSqlTranslatingExpressionVisitor.cs

+                _ => visitedExpression
+            };
+
+            string ResolveFunctionNameFromExpressionType(ExpressionType expressionType)


lokalmatador · 2020-05-17T07:15:36Z

We need functional tests which performs decimal operations in query and assert generated SQL with it.

@smitpatel Who will add those? Is it supposed to be me? If yes, maybe you can elaborate a little more on it?

smitpatel · 2020-05-18T17:49:30Z

@maumar - Can you help with testing here?

smitpatel · 2020-05-27T19:07:41Z

Ping @maumar

maumar · 2020-07-09T03:36:22Z

@lokalmatador sorry for the super late reply. For the test, please add the following snippet BuiltInDataTypesSqliteTest (around line 1508, just before AssertTranslationFailed helper method):

        [ConditionalFact]
        public virtual void Projecting_aritmetic_operations_on_decimals()
        {
            using var context = CreateContext();
            var expected = (from dt1 in context.Set<BuiltInDataTypes>().ToList()
                            from dt2 in context.Set<BuiltInDataTypes>().ToList()
                            orderby dt1.Id, dt2.Id
                            select new
                            {
                                add = dt1.TestDecimal + dt2.TestDecimal,
                                subtract = dt1.TestDecimal - dt2.TestDecimal,
                                multiply = dt1.TestDecimal * dt2.TestDecimal,
                                divide = dt1.TestDecimal / dt2.TestDecimal,
                                negate = -dt1.TestDecimal
                            }).ToList();

            Fixture.TestSqlLoggerFactory.Clear();

            var actual = (from dt1 in context.Set<BuiltInDataTypes>()
                          from dt2 in context.Set<BuiltInDataTypes>()
                          orderby dt1.Id, dt2.Id
                          select new
                          {
                              add = dt1.TestDecimal + dt2.TestDecimal,
                              subtract = dt1.TestDecimal - dt2.TestDecimal,
                              multiply = dt1.TestDecimal * dt2.TestDecimal,
                              divide = dt1.TestDecimal / dt2.TestDecimal,
                              negate = -dt1.TestDecimal
                          }).ToList();

            Assert.Equal(expected.Count, actual.Count);
            for (var i = 0; i < expected.Count; i++)
            {
                Assert.Equal(expected[i].add, actual[i].add);
                Assert.Equal(expected[i].subtract, actual[i].subtract);
                Assert.Equal(expected[i].multiply, actual[i].multiply);
                Assert.Equal(expected[i].divide, actual[i].divide);
                Assert.Equal(expected[i].negate, actual[i].negate);
            }

            AssertSql(
                @"SELECT ef_add(""b"".""TestDecimal"", ""b0"".""TestDecimal"") AS ""add"", ef_add(""b"".""TestDecimal"", ef_negate(""b0"".""TestDecimal"")) AS ""subtract"", ef_multiply(""b"".""TestDecimal"", ""b0"".""TestDecimal"") AS ""multiply"", ef_divide(""b"".""TestDecimal"", ""b0"".""TestDecimal"") AS ""divide"", ef_negate(""b"".""TestDecimal"") AS ""negate""
FROM ""BuiltInDataTypes"" AS ""b""
CROSS JOIN ""BuiltInDataTypes"" AS ""b0""
ORDER BY ""b"".""Id"", ""b0"".""Id""");
        }

Also, now some tests in NorthwindAggregateOperatorsQuerySqliteTest are failing with different exception (we used to throw translation error now we translate successfully but sqlite throws NSE. Please replace NorthwindAggregateOperatorsQuerySqliteTest listing the with the following:

// 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.

using System;
using System.Threading.Tasks;
using Microsoft.EntityFrameworkCore.TestUtilities;
using Xunit;
using Xunit.Abstractions;

namespace Microsoft.EntityFrameworkCore.Query
{
    public class NorthwindAggregateOperatorsQuerySqliteTest : NorthwindAggregateOperatorsQueryRelationalTestBase<NorthwindQuerySqliteFixture<NoopModelCustomizer>>
    {
        public NorthwindAggregateOperatorsQuerySqliteTest(NorthwindQuerySqliteFixture<NoopModelCustomizer> fixture, ITestOutputHelper testOutputHelper)
            : base(fixture)
        {
            ClearLog();
            //Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
        }

        public override Task Sum_with_division_on_decimal(bool async)
            => Assert.ThrowsAsync<NotSupportedException>(() => base.Sum_with_division_on_decimal(async));

        public override Task Sum_with_division_on_decimal_no_significant_digits(bool async)
            => Assert.ThrowsAsync<NotSupportedException>(() => base.Sum_with_division_on_decimal_no_significant_digits(async));

        public override Task Average_with_division_on_decimal(bool async)
            => Assert.ThrowsAsync<NotSupportedException>(() => base.Average_with_division_on_decimal(async));

        public override Task Average_with_division_on_decimal_no_significant_digits(bool async)
            => Assert.ThrowsAsync<NotSupportedException>(() => base.Average_with_division_on_decimal_no_significant_digits(async));
    }
}

and I will merge the PR

smitpatel · 2020-07-09T04:32:52Z

Investigate NSE please

lokalmatador · 2020-07-09T06:01:55Z

@maumar thanks for the code snippets, @smitpatel will do so!

maumar · 2020-07-09T10:12:28Z

in TranslateSum/TranslateAverage we check for the argument type to not be decimal. If it is, we throw the NSE that gets surfaced here. When I removed the check, test passes, but not sure if there are some more complicated cases that we are protecting against. @bricelam - thoughts?

lokalmatador · 2020-07-09T20:19:58Z

Test are now passing. No exceptions.

maumar · 2020-07-10T19:26:01Z

@lokalmatador I have merged the PR. Thank you for the contribution!

lokalmatador · 2020-07-10T19:45:51Z

You're welcome!

bricelam · 2020-07-10T21:56:37Z

@maumar The rest of #19635 is about handling aggregates and OrderBy

lokalmatador added 9 commits March 6, 2020 15:46

add ef_negate

2f8e75c

Merge branch 'master' into basic_arithmetics

45ae829

add ef_add

d358ee3

add ef_divide

37b4752

add ef_multiply

729af46

add "ef_subtract" by ef_add and negated subtrahend

430693a

refactoring

4737cc9

Merge branch 'master' into basic_arithmetics

bd945f0

renaming

9281828

lokalmatador added 2 commits March 7, 2020 18:10

refactoring

76185de

renaming

f72fcb5

ajcvickers assigned bricelam Mar 9, 2020

lokalmatador mentioned this pull request Mar 13, 2020

SQLite: Enable Decimal #19635

Open

bricelam approved these changes Mar 13, 2020

View reviewed changes

fix: Apply bricelam's comments.

e914ab9

bricelam approved these changes Mar 14, 2020

View reviewed changes

Merge branch 'master' into basic_arithmetics

4c29b8a

lokalmatador marked this pull request as ready for review March 29, 2020 12:09

smitpatel reviewed Apr 20, 2020

View reviewed changes

lokalmatador added 4 commits April 23, 2020 18:16

Merge branch 'master' into basic_arithmetics

d5842ba

new:

18afdff

Merge remote-tracking branch 'upstream/master' into basic_arithmetics

9284d0b

fix: call SqlExpressionFactory from Dependencies

531e1e6

bricelam assigned maumar and unassigned bricelam Jun 9, 2020

Merge branch 'master' into basic_arithmetics

8c436d9

lokalmatador added 2 commits July 9, 2020 18:16

Merge branch 'master' into basic_arithmetics

24133b0

[new] update tests

35822f8

maumar merged commit 30bb611 into dotnet:master Jul 10, 2020

ajcvickers mentioned this pull request Jul 17, 2020

Entity Framework Weekly Status Updates (2020) #19549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic arithmetics #20212

Basic arithmetics #20212

lokalmatador commented Mar 7, 2020

lokalmatador commented Mar 7, 2020 •

edited

Loading

bricelam Mar 13, 2020

lokalmatador Mar 14, 2020

bricelam Mar 13, 2020

lokalmatador Mar 14, 2020

bricelam Mar 13, 2020

lokalmatador Mar 14, 2020

bricelam Mar 13, 2020

lokalmatador Mar 14, 2020

bricelam Mar 13, 2020

lokalmatador Mar 14, 2020

lokalmatador commented Mar 14, 2020 •

edited

Loading

lokalmatador commented Mar 21, 2020

smitpatel commented Mar 21, 2020

lokalmatador commented Mar 21, 2020

lokalmatador commented Apr 19, 2020

smitpatel left a comment

smitpatel Apr 20, 2020

lokalmatador May 3, 2020

lokalmatador commented May 17, 2020

smitpatel commented May 18, 2020

smitpatel commented May 27, 2020

maumar commented Jul 9, 2020 •

edited

Loading

smitpatel commented Jul 9, 2020

lokalmatador commented Jul 9, 2020 •

edited

Loading

maumar commented Jul 9, 2020 •

edited

Loading

lokalmatador commented Jul 9, 2020

maumar commented Jul 10, 2020

lokalmatador commented Jul 10, 2020

bricelam commented Jul 10, 2020

Basic arithmetics #20212

Basic arithmetics #20212

Conversation

lokalmatador commented Mar 7, 2020

lokalmatador commented Mar 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lokalmatador commented Mar 14, 2020 • edited Loading

lokalmatador commented Mar 21, 2020

smitpatel commented Mar 21, 2020

lokalmatador commented Mar 21, 2020

lokalmatador commented Apr 19, 2020

smitpatel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lokalmatador commented May 17, 2020

smitpatel commented May 18, 2020

smitpatel commented May 27, 2020

maumar commented Jul 9, 2020 • edited Loading

smitpatel commented Jul 9, 2020

lokalmatador commented Jul 9, 2020 • edited Loading

maumar commented Jul 9, 2020 • edited Loading

lokalmatador commented Jul 9, 2020

maumar commented Jul 10, 2020

lokalmatador commented Jul 10, 2020

bricelam commented Jul 10, 2020

lokalmatador commented Mar 7, 2020 •

edited

Loading

lokalmatador commented Mar 14, 2020 •

edited

Loading

maumar commented Jul 9, 2020 •

edited

Loading

lokalmatador commented Jul 9, 2020 •

edited

Loading

maumar commented Jul 9, 2020 •

edited

Loading