-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Fix to #18555 - Query: when rewriting null semantics for comparisons with functions use function specific metadata to get better SQL #19607
Conversation
Outstanding work is adding API and ability for users to annotate custom function with nullability propagation information - will be done in separate PR. Filed #19609 to track this |
Did not at all review this, but a couple of quick questions off the bat:
|
@roji the naming is not the best, I know, and I'd love some suggestions. The actual logic is so that you can specify which exact parameters propagate and which ones don't. There are some cases where we do this selectively, e.g. in Truncate method. wrt COALESCE, this is the only method I'm aware that has this behavior |
Actually, when it comes to methods that can't be null there are a few, e.g. GETDATE (and other parameterless ones), COUNT (as far as I understand) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a bunch of AssertSql's commented out in the SpatialQueryTests that can probably be uncommented now.
src/EFCore.Sqlite.Core/Query/Internal/SqliteByteArrayMethodTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Sqlite.NTS/Query/Internal/SqliteGeometryMemberTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Sqlite.NTS/Query/Internal/SqliteGeometryMethodTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Sqlite.NTS/Query/Internal/SqliteLineStringMemberTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Sqlite.NTS/Query/Internal/SqliteMultiLineStringMemberTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer.NTS/Query/Internal/SqlServerGeometryCollectionMethodTranslator.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer.NTS/Query/Internal/SqlServerGeometryMethodTranslator.cs
Outdated
Show resolved
Hide resolved
9ca9ce7
to
2a8678b
Compare
//FROM [PointEntity] AS [e]"); | ||
AssertSql( | ||
@"SELECT [p].[Id], [p].[Point].AsTextZM() AS [Text] | ||
FROM [PointEntity] AS [p]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤ the beautiful SQL we had in 2.2 has returned
…with functions use function specific metadata to get better SQL When we need to compute whether a function is null, we often can just evaluate nullability of it's constituents (instance & arguments), e.g. SUBSTRING(stringProperty, 0, 5) == null -> stringProperty == null Adding metadata to SqlFunctionExpression: nullResultAllowed - indicates whether function can ever be null, instancePropagatesNullability - indicates whether function instance can be used to calculate nullability of the entire function argumentsPropagateNullability - array indicating which (if any) function arguments can be used to calculate nullability of the entire function If "canBeNull" is set to false we can instantly compute IsNull/IsNotNull of that function. Otherwise, we look at values of instancePropagatesNullability and argumentsPropagateNullability - if any of them are set to true, we use corresponding argument(s) to compute function nullability. If all of them are set to false we must fallback to the old method and evaluate nullability of the entire function. Resolves #18555
[CanBeNull] string schema, | ||
[NotNull] string name, | ||
[NotNull] IEnumerable<SqlExpression> arguments, | ||
bool nullResultAllowed, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
canBeNull would be better name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will go with nullable/isnullable, just like column expression
nullabilityPropagationElements.Add(sqlFunctionExpression.Instance); | ||
} | ||
|
||
for (var i = 0; i < sqlFunctionExpression.Arguments.Count; i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if is niladic to avoid for loop.
|
||
if (nullabilityPropagationElements.Count > 0) | ||
{ | ||
var result = ProcessNullNotNull( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using aggregate could be cleaner.
@@ -56,6 +56,8 @@ public RelationalMethodCallTranslatorProvider([NotNull] RelationalMethodCallTran | |||
dbFunction.Schema, | |||
dbFunction.Name, | |||
arguments, | |||
nullResultAllowed: true, | |||
argumentsPropagateNullability: arguments.Select(a => true).ToList(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default should be false.
@@ -485,7 +488,52 @@ public virtual CaseExpression Case(IReadOnlyList<CaseWhenClause> whenClauses, Sq | |||
} | |||
|
|||
public virtual SqlFunctionExpression Function( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obsolete old .Function which does not take nullability propagation arguments.
@@ -17,17 +17,80 @@ public class SqlFunctionExpression : SqlExpression | |||
[NotNull] string name, | |||
[NotNull] Type type, | |||
[CanBeNull] RelationalTypeMapping typeMapping) | |||
=> CreateNiladic(name, nullResultAllowed: true, type, typeMapping); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise obsolete ctors
We need to remove this static methods also. Causing unnecessary explosion at low value. We need a better mechanism to support user defined translations anyway.
@@ -93,7 +97,12 @@ public override SqlExpression TranslateLongCount(Expression expression = null) | |||
} | |||
|
|||
return SqlExpressionFactory.ApplyDefaultTypeMapping( | |||
SqlExpressionFactory.Function("COUNT_BIG", new[] { SqlExpressionFactory.Fragment("*") }, typeof(long))); | |||
SqlExpressionFactory.Function( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Count cannot be null then can Count_Big be null?
When we need to compute whether a function is null, we often can just evaluate nullability of it's constituents (instance & arguments), e.g.
SUBSTRING(stringProperty, 0, 5) == null -> stringProperty == null
Adding metadata to SqlFunctionExpression:
canBeNull - indicates whether function can ever be null,
instancePropagatesNullability - indicates whether function instance can be used to calculate nullability of the entire function
argumentsPropagateNullability - array indicating which (if any) function arguments can be used to calculate nullability of the entire function
If "canBeNull" is set to false we can instantly compute IsNull/IsNotNull of that function.
Otherwise, we look at values of instancePropagatesNullability and argumentsPropagateNullability - if any of them are set to true, we use corresponding argument(s) to compute function nullability.
If all of them are set to false we must fallback to the old method and evaluate nullability of the entire function.
Resolves #18555