Skip to content

Refactor projection binding to avoid redundant Convert nodes and impr… #37625

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.

Sign up for GitHub

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

Open
Abde1rahman1 wants to merge 4 commits into
base: main
Choose a base branch
from
+74 −23
Open

Refactor projection binding to avoid redundant Convert nodes and impr… #37625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d2ad26f
Refactor projection binding to avoid redundant Convert nodes and impr…
Abde1rahman1 Feb 4, 2026
cbe518d
update Visit method in RelationalProjectionBindingExpressionVisitor c…
Abde1rahman1 Feb 4, 2026
3c37b6f
second update
Abde1rahman1 Feb 4, 2026
1d92788
solve errors
Abde1rahman1 Feb 4, 2026
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: 2 additions & 2 deletions src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ public virtual Expression Translate(SelectExpression selectExpression, Expressio
}

return new ProjectionBindingExpression(
_selectExpression, _selectExpression.AddToProjection(translation), expression.Type.MakeNullable());
_selectExpression, _selectExpression.AddToProjection(translation), expression.Type);
}
else
{
Expand All @@ -148,7 +148,7 @@ public virtual Expression Translate(SelectExpression selectExpression, Expressio

_projectionMapping[_projectionMembers.Peek()] = translation;

return new ProjectionBindingExpression(_selectExpression, _projectionMembers.Peek(), expression.Type.MakeNullable());
return new ProjectionBindingExpression(_selectExpression, _projectionMembers.Peek(), expression.Type);
}
}

Expand Down
55 changes: 35 additions & 20 deletions src/EFCore.Relational/Query/Internal/RelationalProjectionBindingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,13 +120,19 @@ public virtual Expression Translate(SelectExpression selectExpression, Expressio
case ParameterExpression parameterExpression:
throw new InvalidOperationException(CoreStrings.TranslationFailed(parameterExpression.Print()));

case ProjectionBindingExpression projectionBindingExpression:
return _selectExpression.GetProjection(projectionBindingExpression) switch
{
StructuralTypeProjectionExpression projection => AddClientProjection(projection, typeof(ValueBuffer)),
SqlExpression mappedSqlExpression => AddClientProjection(mappedSqlExpression, expression.Type.MakeNullable()),
_ => throw new InvalidOperationException(CoreStrings.TranslationFailed(projectionBindingExpression.Print()))
};
case SqlExpression mappedSqlExpression:
var isNullable = mappedSqlExpression switch
{
ColumnExpression c => c.IsNullable,
SqlFunctionExpression f => f.IsNullable,
_ => true
};

return AddClientProjection(
mappedSqlExpression,
isNullable && expression.Type.IsValueType && !expression.Type.IsNullableType()
? expression.Type.MakeNullable()
: expression.Type);

case MaterializeCollectionNavigationExpression materializeCollectionNavigationExpression:
if (materializeCollectionNavigationExpression.Navigation.TargetEntityType.IsMappedToJson())
Expand Down Expand Up @@ -233,9 +239,20 @@ public virtual Expression Translate(SelectExpression selectExpression, Expressio
switch (_sqlTranslator.TranslateProjection(expression))
{
case SqlExpression sqlExpression:
_projectionMapping[_projectionMembers.Peek()] = sqlExpression;
return new ProjectionBindingExpression(
_selectExpression, _projectionMembers.Peek(), expression.Type.MakeNullable());
_projectionMapping[_projectionMembers.Peek()] = sqlExpression;
var isNullable = sqlExpression switch
{
ColumnExpression c => c.IsNullable,
SqlFunctionExpression f => f.IsNullable,
_ => true
};

return new ProjectionBindingExpression(
_selectExpression,
_projectionMembers.Peek(),
isNullable && expression.Type.IsValueType && !expression.Type.IsNullableType()
? expression.Type.MakeNullable()
: expression.Type);

// This handles the case of a complex type being projected out of a Select.
// Note that an entity type being projected is (currently) handled differently
Expand Down Expand Up @@ -679,18 +696,16 @@ private static Expression MatchTypes(Expression expression, Type targetType)
if (targetType != expression.Type
&& targetType.TryGetElementType(typeof(IQueryable<>)) == null)
{
Check.DebugAssert(
targetType.MakeNullable() == expression.Type,
$"expression has type {expression.Type.Name}, but must be nullable over {targetType.Name}");

return expression switch
if (expression is ProjectionBindingExpression projectionBindingExpression)
{
#pragma warning disable EF1001
RelationalStructuralTypeShaperExpression structuralShaper => structuralShaper.MakeClrTypeNonNullable(),
#pragma warning restore EF1001
return projectionBindingExpression.UpdateType(targetType);
}

_ => Expression.Convert(expression, targetType),
};
if (expression is RelationalStructuralTypeShaperExpression structuralShaper)
{
return structuralShaper.MakeClrTypeNonNullable();
}
return Expression.Convert(expression, targetType);
}

return expression;
Expand Down
16 changes: 16 additions & 0 deletions src/EFCore/Query/ProjectionBindingExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,22 @@ void IPrintableExpression.Print(ExpressionPrinter expressionPrinter)
expressionPrinter.Append(Index.ToString()!);
}
}
/// <summary>
/// Creates a new instance of the <see cref="ProjectionBindingExpression" /> class with a new type.
/// </summary>
/// <param name="type">The new clr type of value being read.</param>
/// <returns>A new projection binding expression with the updated type.</returns>
public virtual ProjectionBindingExpression UpdateType(Type type)
{
if (type == Type)
{
return this;
}

return Index != null
? new ProjectionBindingExpression(QueryExpression, Index.Value, type)
: new ProjectionBindingExpression(QueryExpression, ProjectionMember!, type);
}

/// <inheritdoc />
public override bool Equals(object? obj)
Expand Down
22 changes: 21 additions & 1 deletion test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSqlQuerySqlServerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.Data.SqlClient;

using Microsoft.EntityFrameworkCore.TestModels.Northwind;
namespace Microsoft.EntityFrameworkCore.Query;

#nullable disable
Expand Down Expand Up @@ -70,6 +70,26 @@ public override async Task SqlQuery_over_int_with_parameter(bool async)
""");
}

[ConditionalFact]
public virtual void Projection_binding_clean_up_non_nullable_value_type()
{
using var context = Fixture.CreateContext();

var query = context.Set<Customer>()
.Select(c => c.CustomerID)
.ToQueryString();
var result = context.Set<Customer>().Select(c => c.CustomerID).ToList();
Assert.NotEmpty(result);
}

[ConditionalFact]
public virtual void Projection_binding_stays_nullable_for_nullable_types()
{
using var context = Fixture.CreateContext();
var result = context.Set<Employee>().Select(e => e.ReportsTo).ToList();

Assert.Contains(null, result);
}
protected override DbParameter CreateDbParameter(string name, object value)
=> new SqlParameter { ParameterName = name, Value = value };

Expand Down
Loading