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

UseBinaryAggregateExpression rework #812

Merged
merged 19 commits into from
Aug 30, 2017
Merged

UseBinaryAggregateExpression rework #812

merged 19 commits into from
Aug 30, 2017

Conversation

MaceWindu
Copy link
Contributor

No description provided.

@MaceWindu
Copy link
Contributor Author

@detoxhby I hope it will solve your issue

@ili ili mentioned this pull request Aug 19, 2017
42 tasks
@MaceWindu MaceWindu added this to the 1.9.0 milestone Aug 19, 2017
@MaceWindu
Copy link
Contributor Author

Found another issue with this functionality - query cache comparison uses initial expression and still fails

@detoxhby
Copy link

detoxhby commented Aug 19, 2017

Thanks, tested with 7e05a48

@detoxhby
Copy link

detoxhby commented Aug 19, 2017

Have to correct myself, I forgot that I've removed the LinqToDB.Common.Configuration.Linq.UseBinaryAggregateExpression = true for previous tests and ran without it. Setting it true again fails the same way. ⛔ (tested with 683cedf)

Cannot convert from „System.Linq.Expressions.PropertyExpression” to „System.Linq.Expressions.BinaryExpression” type

lambda_method(Closure , Expression , Object[] )
LinqToDB.Linq.QueryRunner.SetParameters(Query query, IDataContext dataContext, Expression expression, Object[] parameters, Int32 queryNumber) in: C:\dev\ext\linq2db\Source\Linq\QueryRunner.cs, line: 154
LinqToDB.Linq.QueryRunnerBase.SetCommand(Boolean clearQueryHints) in: C:\dev\ext\linq2db\Source\Linq\QueryRunnerBase.cs, line: 74
LinqToDB.Data.DataConnection.QueryRunner.ExecuteReader() in: C:\dev\ext\linq2db\Source\Data\DataConnection.QueryRunner.cs, line: 420
LinqToDB.Linq.QueryRunner.<ExecuteQuery>d__15`1.MoveNext() in: C:\dev\ext\linq2db\Source\Linq\QueryRunner.cs, line: 327
System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)

@MaceWindu
Copy link
Contributor Author

I have good news.
I've enabled this option for all tests and now it reproduces pretty stable on two tests (TypesTests.Bool1 and Issue271Tests.Test2). This means I can debug and fix it.

@MaceWindu
Copy link
Contributor Author

MaceWindu commented Aug 19, 2017

@detoxhby could you test it once more?
Update: looks like it is not correct fix as it broke some tests

@detoxhby
Copy link

detoxhby commented Aug 19, 2017

@MaceWindu just did it, something broke. Random errors from not inserting data to even missing FKs :/

(just read your update)

@MaceWindu
Copy link
Contributor Author

@detoxhby now it should work

@detoxhby
Copy link

@MaceWindu seems good! ✅

Even though the commit diff looks terrifying ... was it really "just" that? How was it working for other providers (like SqlServer) and struggle only on SQLCe if there is no provider-specific changes here?
Don't get me wrong, I'm just curious and trying to understand better.

@MaceWindu
Copy link
Contributor Author

MaceWindu commented Aug 19, 2017

Well, it wasn't SqlCe specific issue. For queries with parameters we generate methods to extract parameter value from expression. They were generated to work with initial expression, but when this optimization applied - expression structure changes and parameter extractor receives expression with different structure and that is why you get all those invalid cast expressions: when method expected element of one type - he received completely different one.

Why it was broken only for SqlCe in your case - I cannot say for sure. It suspect one of those:

  • expression wasn't optimized
  • optimized expression still had structure, compatible with parameter extraction method

With this fix, expression will be optimized before it used by linq2db for anything else.

Update: also issue with incorrect query caching, fixed with this PR, also was affecting chances to get this error.

@detoxhby
Copy link

@MaceWindu thanks for the explanation and fix, keep up the good work!

@@ -334,7 +331,30 @@ Expression AggregateExpression(Expression expression)

if (items.Count > 2)
Copy link
Member

@sdanyliv sdanyliv Aug 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use here Count > 3

@ili ili merged commit 17e5be6 into master Aug 30, 2017
@ili ili deleted the fix_1_9_0_tests branch August 30, 2017 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants