Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Translation error using negative #13919

Closed
dotnetshadow opened this issue Nov 8, 2018 · 7 comments
Closed

Translation error using negative #13919

dotnetshadow opened this issue Nov 8, 2018 · 7 comments
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported good first issue This issue should be relatively straightforward to fix. poachable punted-for-3.0 type-bug
Milestone

Comments

@dotnetshadow
Copy link

dotnetshadow commented Nov 8, 2018

Trying to sum a list of numbers and apply a negative value to it.
Seems like a translation issue?

Steps to reproduce

Summing up a list of rows in a transaction table
Basically I needed the negative value of a sum, it turns out it was actually giving me -180 instead of -220
Instead I did -1 * (1 * 200 + 20) and it gave the right result not sure why?

_db.Transactions.Select(x=> x.Type == MyEnum.Type1 ?  -(price * units + bonus) : 0)
_db.Transactions.Select(x=> x.Type == MyEnum.Type1 ?  -(1 * 200 + 20) : 0)

Using a linq sample gives the correct values

static void Main(string[] args)
        {
            var i = -(1 * 200 + 20);

            Console.WriteLine($"{i}");
        }

Further technical details

EF Core version: 2.1.4
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 10
IDE: Visual Studio 2017 15.8.9

@tuespetre
Copy link
Contributor

@smitpatel @maumar it looks like in DefaultQuerySqlGenerator, VisitUnary visits ExpressionType.Negate by appending the hyphen and then visiting the operand. Probably needs to add parenthesis before and after visiting the operand.

@poke
Copy link

poke commented Nov 10, 2018

For what it’s worth, the in-memory database does not have this problem (which is why my own attempts to reproduce this failed 😅)

@dotnetshadow
Copy link
Author

dotnetshadow commented Nov 14, 2018

@poke @tuespetre Actually I just realised that I'm using Z.EntityFramework.Plus Future query feature which might have the issue. I have opened up an issue there in case it's their issue more than entity framework?

var transactionQuery = _context.Transactions.AsNoTracking().Where(x => x.Id == id)
                .DeferredSum(
                    x => x.TradeType == TradeType.Buy ? -(x.Units * x.Stock.StockPrice.Last + x.Brokerage) : x.TradeType == TradeType.Sell ? x.Units * x.Stock.StockPrice.Last - x.Brokerage : 0)
                .FutureValue<decimal>();

@poke
Copy link

poke commented Nov 14, 2018

No, I was able to reproduce the problem eventually with the SQLite provider.

@dotnetshadow
Copy link
Author

@poke Ahh that's awesome news, cheers hopefully the EF team will get on top of this. Could be pretty critical for some apps

@ajcvickers ajcvickers added this to the 3.0.0 milestone Nov 14, 2018
@ajcvickers ajcvickers modified the milestones: 3.0.0, Backlog Jun 28, 2019
@smitpatel smitpatel removed their assignment Aug 7, 2019
@smitpatel
Copy link
Member

Probably this still exist in new pipeline. Also look for other operatorType in SqlUnary.

@smitpatel smitpatel added the good first issue This issue should be relatively straightforward to fix. label Nov 22, 2019
@jonlouie
Copy link
Contributor

jonlouie commented Jun 22, 2020

@smitpatel I would like to contribute to the EF Core project and believe this issue is a suitable entry point. I took a look and was able to verify that this issue still exists for SQL Server and SQLite. Is it currently up for grabs?

@maumar maumar closed this as completed in 4795eee Jun 30, 2020
@maumar maumar modified the milestones: Backlog, 5.0.0 Jun 30, 2020
@maumar maumar added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jun 30, 2020
maumar pushed a commit that referenced this issue Jul 1, 2020
create negation unit tests

add parentheses to negated expression on translation

expose unit test to sqlite test suite

add unit tests for negating a column and LIKE expression

use RequiresBrackets helper

Fixes #13919
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview8 Jul 14, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview8, 5.0.0 Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported good first issue This issue should be relatively straightforward to fix. poachable punted-for-3.0 type-bug
Projects
None yet
Development

No branches or pull requests

7 participants