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

Eliminate nullability-only Convert nodes #16653

Closed
roji opened this issue Jul 18, 2019 · 2 comments · Fixed by #16655 or #16746
Closed

Eliminate nullability-only Convert nodes #16653

roji opened this issue Jul 18, 2019 · 2 comments · Fixed by #16655 or #16746
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@roji
Copy link
Member

roji commented Jul 18, 2019

Given the following query:

await AssertQuery<Order>(isAsync, os =>
    os.Select(o => new Order { OrderDate = o.OrderDate.Value }));

we get the following tree (since OrderDate is nullable):

value(Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryable`1[Microsoft.EntityFrameworkCore.TestModels.Northwind.Order]).Select(o => new Order() {OrderDate = Convert(o.OrderDate.Value, Nullable`1)})

which makes us generate the following over-complicated SQL:

SELECT CAST(o."OrderDate" AS timestamp without time zone) AS "OrderDate" FROM "Orders" AS o

FWIW I think the old pipeline didn't have this useless cast.

@roji roji added the query label Jul 18, 2019
@smitpatel
Copy link
Contributor

smitpatel commented Jul 18, 2019

https://github.com/aspnet/EntityFrameworkCore/blob/master/src/EFCore.Relational/Query/Pipeline/RelationalSqlTranslatingExpressionVisitor.cs#L543

There is already check in place to make sure if the convert node is converting from T to T? we remove it.

@smitpatel
Copy link
Contributor

Fix reverted due to un-updated SQL baselines.

@ajcvickers ajcvickers added this to the 3.0.0 milestone Jul 19, 2019
roji added a commit that referenced this issue Jul 25, 2019
roji added a commit that referenced this issue Jul 25, 2019
@roji roji added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jul 26, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview8 Jul 29, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0-preview8, 3.0.0 Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
3 participants