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

Allow set operations for columns that map to same store type #30400

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/EFCore.Relational/Query/QuerySqlGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -615,9 +615,7 @@ protected override Expression VisitSqlParameter(SqlParameterExpression sqlParame
p =>
p.InvariantName == parameterName
&& p is TypeMappedRelationalParameter typeMappedRelationalParameter
&& string.Equals(
typeMappedRelationalParameter.RelationalTypeMapping.StoreType, sqlParameterExpression.TypeMapping!.StoreType,
StringComparison.OrdinalIgnoreCase)
&& typeMappedRelationalParameter.RelationalTypeMapping.MapsToSameStoreType(sqlParameterExpression.TypeMapping!)
&& typeMappedRelationalParameter.RelationalTypeMapping.Converter == sqlParameterExpression.TypeMapping!.Converter);

if (parameter is null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2334,10 +2334,7 @@ private void ApplySetOperation(SetOperationType setOperationType, SelectExpressi
var innerColumn2 = (SqlExpression)expression2;
// For now, make sure that both sides output the same store type, otherwise the query may fail.
// TODO: with #15586 we'll be able to also allow different store types which are implicitly convertible to one another.
if (!string.Equals(
innerColumn1.TypeMapping!.StoreType,
innerColumn2.TypeMapping!.StoreType,
StringComparison.OrdinalIgnoreCase))
if (!innerColumn1.TypeMapping!.MapsToSameStoreType(innerColumn2.TypeMapping!))
{
throw new InvalidOperationException(RelationalStrings.SetOperationsOnDifferentStoreTypes);
}
Expand Down
18 changes: 18 additions & 0 deletions src/EFCore.Relational/Storage/RelationalTypeMapping.cs
Original file line number Diff line number Diff line change
Expand Up @@ -627,4 +627,22 @@ public static MethodInfo GetDataReaderMethod(Type type)
/// <returns>The expression with customization added.</returns>
public virtual Expression CustomizeDataReaderExpression(Expression expression)
=> expression;

/// <inheritdoc />
public override bool MapsToSameStoreType(CoreTypeMapping otherMapping)
{
if (!base.MapsToSameStoreType(otherMapping))
{
return false;
}

var otherRelationalMapping = ((RelationalTypeMapping)otherMapping);
return
Size == otherRelationalMapping.Size
&& Precision == otherRelationalMapping.Precision
&& DbType == otherRelationalMapping.DbType
&& Scale == otherRelationalMapping.Scale
&& IsUnicode == otherRelationalMapping.IsUnicode
&& IsFixedLength == otherRelationalMapping.IsFixedLength;
}
}
10 changes: 10 additions & 0 deletions src/EFCore/Storage/CoreTypeMapping.cs
Original file line number Diff line number Diff line change
Expand Up @@ -224,4 +224,14 @@ public virtual ValueComparer ProviderValueComparer
/// <returns>An expression tree that can be used to generate code for the literal value.</returns>
public virtual Expression GenerateCodeLiteral(object value)
=> throw new NotSupportedException(CoreStrings.LiteralGenerationNotSupported(ClrType.ShortDisplayName()));

/// <summary>
/// Determines whether this type mapping is to the same store type as the given mapping.
/// </summary>
/// <param name="otherMapping">The other type mapping.</param>
/// <returns><see langword="true"/> if the two mappings map to the same type, <see langword="false"/> otherwise.</returns>
public virtual bool MapsToSameStoreType(CoreTypeMapping otherMapping)
Copy link
Member

Choose a reason for hiding this comment

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

Should this only compare the actual (string) store type name into account? There could be different mappings - including even with different CLR types - which map to the same store type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind; I think you are right. Will re-work this. Was trying to be too clever. Always best not to try to be clever.

=> GetType() == otherMapping.GetType()
&& (Converter?.ProviderClrType ?? ClrType)
== (otherMapping.Converter?.ProviderClrType ?? otherMapping.ClrType);
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
b.Property(c => c.CustomerID).HasColumnType("nchar(5)");
b.Property(cm => cm.CompanyName).HasMaxLength(40);
b.Property(cm => cm.ContactName).HasMaxLength(30);
b.Property(cm => cm.ContactTitle).HasColumnType("NVarChar(30)");
b.Property(cm => cm.ContactTitle).HasColumnType("national character varying(30)");
});

modelBuilder.Entity<Employee>(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1344,7 +1344,7 @@ public virtual async Task Union_with_different_store_types_throws(bool async)

[ConditionalTheory] // Issue #29020
[MemberData(nameof(IsAsyncData))]
public virtual async Task Union_with_store_types_differing_only_by_case(bool async)
public virtual async Task Union_with_type_mappings_to_same_store_type(bool async)
{
await AssertQuery(
async,
Expand Down