-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Simplify COALESCE based on the nullability of its arguments
#33938
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
Changes from all commits
1d44c17
91c269a
88f596a
f152204
8861ada
6a3488a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,7 +88,7 @@ private SqlFunctionExpression( | |
| Type type, | ||
| RelationalTypeMapping? typeMapping) | ||
| : this( | ||
| instance, schema, name, niladic: true, arguments: null, nullable, instancePropagatesNullability, | ||
| instance, schema, name, arguments: null, nullable, instancePropagatesNullability, | ||
| argumentsPropagateNullability: null, builtIn, type, typeMapping) | ||
| { | ||
| } | ||
|
|
@@ -165,24 +165,6 @@ public SqlFunctionExpression( | |
| { | ||
| } | ||
|
|
||
| private SqlFunctionExpression( | ||
| SqlExpression? instance, | ||
| string? schema, | ||
| string name, | ||
| IEnumerable<SqlExpression> arguments, | ||
| bool nullable, | ||
| bool? instancePropagatesNullability, | ||
| IEnumerable<bool> argumentsPropagateNullability, | ||
| bool builtIn, | ||
| Type type, | ||
| RelationalTypeMapping? typeMapping) | ||
| : this( | ||
| instance, schema, name, niladic: false, arguments, nullable, | ||
| instancePropagatesNullability, argumentsPropagateNullability, builtIn, | ||
| type, typeMapping) | ||
| { | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to | ||
| /// the same compatibility standards as public APIs. It may be changed or removed without notice in | ||
|
|
@@ -194,7 +176,6 @@ public SqlFunctionExpression( | |
| SqlExpression? instance, | ||
| string? schema, | ||
| string name, | ||
| bool niladic, | ||
| IEnumerable<SqlExpression>? arguments, | ||
| bool nullable, | ||
| bool? instancePropagatesNullability, | ||
|
|
@@ -207,7 +188,6 @@ public SqlFunctionExpression( | |
| Instance = instance; | ||
| Name = name; | ||
| Schema = schema; | ||
| IsNiladic = niladic; | ||
| IsBuiltIn = builtIn; | ||
| Arguments = arguments?.ToList(); | ||
| IsNullable = nullable; | ||
|
|
@@ -240,7 +220,7 @@ public SqlFunctionExpression( | |
| /// A bool value indicating if the function is niladic. | ||
| /// </summary> | ||
| [MemberNotNullWhen(false, nameof(Arguments), nameof(ArgumentsPropagateNullability))] | ||
| public virtual bool IsNiladic { get; } | ||
| public virtual bool IsNiladic => Arguments is null; | ||
|
|
||
| /// <summary> | ||
| /// A bool value indicating if the function is built-in. | ||
|
|
@@ -295,7 +275,6 @@ protected override Expression VisitChildren(ExpressionVisitor visitor) | |
| instance, | ||
| Schema, | ||
| Name, | ||
| IsNiladic, | ||
| arguments, | ||
| IsNullable, | ||
| InstancePropagatesNullability, | ||
|
|
@@ -316,7 +295,6 @@ public virtual SqlFunctionExpression ApplyTypeMapping(RelationalTypeMapping? typ | |
| Instance, | ||
| Schema, | ||
| Name, | ||
| IsNiladic, | ||
| Arguments, | ||
| IsNullable, | ||
| InstancePropagatesNullability, | ||
|
|
@@ -333,33 +311,51 @@ public virtual SqlFunctionExpression ApplyTypeMapping(RelationalTypeMapping? typ | |
| /// <param name="arguments">The <see cref="Arguments" /> property of the result.</param> | ||
| /// <returns>This expression if no children changed, or an expression with the updated children.</returns> | ||
| public virtual SqlFunctionExpression Update(SqlExpression? instance, IReadOnlyList<SqlExpression>? arguments) | ||
| => instance != Instance || (arguments != null && Arguments != null && !arguments.SequenceEqual(Arguments)) | ||
| ? new SqlFunctionExpression( | ||
| => Update(instance, arguments, ArgumentsPropagateNullability); | ||
|
|
||
| /// <summary> | ||
| /// Creates a new expression that is like this one, but using the supplied children. If all of the children are the same, it will | ||
| /// return this expression. | ||
| /// </summary> | ||
| /// <param name="instance">The <see cref="Instance" /> property of the result.</param> | ||
| /// <param name="arguments">The <see cref="Arguments" /> property of the result.</param> | ||
| /// <param name="argumentsPropagateNullability">The <see cref="ArgumentsPropagateNullability" /> property of the result. If omitted, | ||
| /// the current ArgumentsPropagateNullability will be used.</param> | ||
| /// <returns>This expression if no children changed, or an expression with the updated children.</returns> | ||
| public virtual SqlFunctionExpression Update( | ||
| SqlExpression? instance, | ||
| IReadOnlyList<SqlExpression>? arguments, | ||
| IReadOnlyList<bool>? argumentsPropagateNullability) | ||
| => instance == Instance | ||
| && ((arguments == Arguments) || (arguments != null && Arguments != null && arguments.SequenceEqual(Arguments))) | ||
| && ((argumentsPropagateNullability == ArgumentsPropagateNullability) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, the problem this creates is that argumentsPropagateNullability being null could mean two things: either the user is switching to niladic (and so arguments is null too), or they want to just change the arguments without changing argumentsPropagateNullability (which seems like a common scenario). Maybe we should just have two overloads, one only for changing arguments (doesn't have argumentsPropagateNullability parameter, and importantly, cannot change the number of arguments), and another with arguments and argumentsPropagateNullability?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll do that (it was indeed my initial approach). |
||
| || (argumentsPropagateNullability != null | ||
| && ArgumentsPropagateNullability != null | ||
| && argumentsPropagateNullability.SequenceEqual(ArgumentsPropagateNullability))) | ||
| ? this | ||
| : new SqlFunctionExpression( | ||
| instance, | ||
| Schema, | ||
| Name, | ||
| IsNiladic, | ||
| arguments, | ||
| IsNullable, | ||
| InstancePropagatesNullability, | ||
| ArgumentsPropagateNullability, | ||
| argumentsPropagateNullability, | ||
| IsBuiltIn, | ||
| Type, | ||
| TypeMapping) | ||
| : this; | ||
| TypeMapping); | ||
|
|
||
| /// <inheritdoc /> | ||
| public override Expression Quote() | ||
| => New( | ||
| _quotingConstructor ??= typeof(SqlFunctionExpression).GetConstructor( | ||
| [ | ||
| typeof(SqlExpression), typeof(string), typeof(string), typeof(bool), typeof(IEnumerable<SqlExpression>), | ||
| typeof(SqlExpression), typeof(string), typeof(string), typeof(IEnumerable<SqlExpression>), | ||
| typeof(bool), typeof(bool), typeof(IEnumerable<bool>), typeof(bool), typeof(Type), typeof(RelationalTypeMapping) | ||
| ])!, | ||
| RelationalExpressionQuotingUtilities.QuoteOrNull(Instance), | ||
| Constant(Schema, typeof(string)), | ||
| Constant(Name), | ||
| Constant(IsNiladic), | ||
| Arguments is null | ||
| ? Constant(null, typeof(IEnumerable<SqlExpression>)) | ||
| : NewArrayInit(typeof(SqlExpression), initializers: Arguments.Select(a => a.Quote())), | ||
|
|
@@ -408,7 +404,6 @@ public override bool Equals(object? obj) | |
|
|
||
| private bool Equals(SqlFunctionExpression sqlFunctionExpression) | ||
| => base.Equals(sqlFunctionExpression) | ||
| && IsNiladic == sqlFunctionExpression.IsNiladic | ||
| && Name == sqlFunctionExpression.Name | ||
| && Schema == sqlFunctionExpression.Schema | ||
| && ((Instance == null && sqlFunctionExpression.Instance == null) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,7 +101,7 @@ public SqlServerStringAggregateMethodTranslator( | |
| source, | ||
| enumerableArgumentIndex: 0, | ||
| nullable: true, | ||
| argumentsPropagateNullability: new[] { false, true }, | ||
| argumentsPropagateNullability: new[] { false, false }, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, good catch. |
||
| typeof(string)), | ||
| _sqlExpressionFactory.Constant(string.Empty, typeof(string)), | ||
| resultTypeMapping); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.