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

SQL Server: Set operations should work if normalized store type is the same #29020

Closed
Deathrage opened this issue Sep 8, 2022 · 7 comments · Fixed by #30468
Closed

SQL Server: Set operations should work if normalized store type is the same #29020

Deathrage opened this issue Sep 8, 2022 · 7 comments · Fixed by #30468
Assignees
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 type-bug
Milestone

Comments

@Deathrage
Copy link

In SelectExpression.cs:1594 (Microsoft.EntityFrameworkCore.Query.SqlExpression) there is code that does

if (innerColumn1.TypeMapping!.StoreType != innerColumn2.TypeMapping!.StoreType)
{
    throw new InvalidOperationException(RelationalStrings.SetOperationsOnDifferentStoreTypes);
}

If col1 type in mode is VARCHAR(20) and col2 type is varchar(20) it throws. This should not happen as SQL server is not case sensitive. I udestrand why it would be desirable for varchar(20) and varchar(50) to throw, but in case of case sensitivity it's exactly the same type.

@Deathrage Deathrage changed the title Comparing StoreType in Union should not be cae sesitive Comparing StoreType in Union should not be case sesitive Sep 8, 2022
@Deathrage Deathrage changed the title Comparing StoreType in Union should not be case sesitive SQL Server: Comparing StoreType in Union should not be case sesitive Sep 8, 2022
@ajcvickers ajcvickers self-assigned this Sep 9, 2022
@ajcvickers ajcvickers added this to the 7.0.0 milestone Sep 9, 2022
@ajcvickers
Copy link
Member

Note from triage: using the store type name is not really appropriate here. For example, it will fail to indicate that "national character varying (20)" is the same as "NVARCHAR(20)".

@roji
Copy link
Member

roji commented Sep 11, 2022

I wonder if we should have a way to normalize store types, yielding their canonical representation which can be compared...

@ajcvickers
Copy link
Member

Note from triage: fix the case-sensitivity for 7.0; punt full fix.

@ajcvickers ajcvickers added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. labels Sep 14, 2022
@ajcvickers ajcvickers changed the title SQL Server: Comparing StoreType in Union should not be case sesitive SQL Server: Set operations should work if normalized store type is the same Sep 14, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0, Backlog Sep 14, 2022
@roji
Copy link
Member

roji commented Nov 23, 2022

When implementing store type normalization, use that also when doing parameter matching in QuerySqlGenerator.VisitSqlParameter (see #29650 (comment)).

@ajcvickers ajcvickers modified the milestones: Backlog, 8.0.0 Mar 3, 2023
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Mar 3, 2023
@ajcvickers
Copy link
Member

Note from design meeting: stop throwing in this case and let the database throw if the types are not compatible.

@ajcvickers
Copy link
Member

Note for triage: we should decide what to do about parameter matching in QuerySqlGenerator.VisitSqlParameter (see #29650 (comment)).

@ajcvickers
Copy link
Member

Note from triage: since the VisitSqlParameter check is an optimization only, the existing check is fine.

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 type-bug
Projects
None yet
3 participants