Fix to #18555 - Query: when rewriting null semantics for comparisons with functions use function specific metadata to get better SQL #19607

maumar · 2020-01-15T23:32:58Z

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

maumar · 2020-01-15T23:35:14Z

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

roji · 2020-01-16T00:47:20Z

Did not at all review this, but a couple of quick questions off the bat:

instancePropagatesNullability and argumentsPropagateNullability seem to indicate an "all or nothing" approach, i.e. it's not possible to say that certain parameters propagate while others don't. Are we sure this is sufficient, are there any counter-examples?
canBeNull: out of curiosity, are we aware of (built-in) functions which cannot be null, aside from COALESCE? Am not doubting the need for this, just curious.

maumar · 2020-01-16T19:30:54Z

@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

maumar · 2020-01-16T20:21:24Z

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)

bricelam

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

src/EFCore.Sqlite.Core/Query/Internal/SqliteDateTimeAddTranslator.cs

src/EFCore.Sqlite.Core/Query/Internal/SqliteStringMethodTranslator.cs

src/EFCore.Sqlite.NTS/Query/Internal/SqliteGeometryMemberTranslator.cs

src/EFCore.Sqlite.NTS/Query/Internal/SqliteGeometryMethodTranslator.cs

src/EFCore.Sqlite.NTS/Query/Internal/SqliteLineStringMemberTranslator.cs

src/EFCore.Sqlite.NTS/Query/Internal/SqliteMultiLineStringMemberTranslator.cs

src/EFCore.SqlServer.NTS/Query/Internal/SqlServerGeometryCollectionMethodTranslator.cs

src/EFCore.SqlServer.NTS/Query/Internal/SqlServerGeometryMethodTranslator.cs

bricelam · 2020-01-22T22:48:12Z

test/EFCore.SqlServer.FunctionalTests/Query/SpatialQuerySqlServerGeographyTest.cs

-//FROM [PointEntity] AS [e]");
+            AssertSql(
+                @"SELECT [p].[Id], [p].[Point].AsTextZM() AS [Text]
+FROM [PointEntity] AS [p]");


❤ 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

smitpatel · 2020-02-06T22:53:13Z

src/EFCore.Relational/Query/ISqlExpressionFactory.cs

+            [CanBeNull] string schema,
+            [NotNull] string name,
+            [NotNull] IEnumerable<SqlExpression> arguments,
+            bool nullResultAllowed,


canBeNull would be better name

will go with nullable/isnullable, just like column expression

smitpatel · 2020-02-06T22:59:44Z

src/EFCore.Relational/Query/NullabilityBasedSqlProcessingExpressionVisitor.cs

+                        nullabilityPropagationElements.Add(sqlFunctionExpression.Instance);
+                    }
+
+                    for (var i = 0; i < sqlFunctionExpression.Arguments.Count; i++)


Check if is niladic to avoid for loop.

smitpatel · 2020-02-06T23:01:42Z

src/EFCore.Relational/Query/NullabilityBasedSqlProcessingExpressionVisitor.cs

+
+                    if (nullabilityPropagationElements.Count > 0)
+                    {
+                        var result = ProcessNullNotNull(


Using aggregate could be cleaner.

smitpatel · 2020-02-06T23:02:28Z

src/EFCore.Relational/Query/RelationalMethodCallTranslatorProvider.cs

@@ -56,6 +56,8 @@ public RelationalMethodCallTranslatorProvider([NotNull] RelationalMethodCallTran
                        dbFunction.Schema,
                        dbFunction.Name,
                        arguments,
+                        nullResultAllowed: true,
+                        argumentsPropagateNullability: arguments.Select(a => true).ToList(),


default should be false.

smitpatel · 2020-02-06T23:04:05Z

src/EFCore.Relational/Query/SqlExpressionFactory.cs

@@ -485,7 +488,52 @@ public virtual CaseExpression Case(IReadOnlyList<CaseWhenClause> whenClauses, Sq
        }

        public virtual SqlFunctionExpression Function(


Obsolete old .Function which does not take nullability propagation arguments.

smitpatel · 2020-02-06T23:07:49Z

src/EFCore.Relational/Query/SqlExpressions/SqlFunctionExpression.cs

@@ -17,17 +17,80 @@ public class SqlFunctionExpression : SqlExpression
            [NotNull] string name,
            [NotNull] Type type,
            [CanBeNull] RelationalTypeMapping typeMapping)
+            => CreateNiladic(name, nullResultAllowed: true, type, typeMapping);


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.

smitpatel · 2020-02-06T23:09:58Z

src/EFCore.SqlServer/Query/Internal/SqlServerSqlTranslatingExpressionVisitor.cs

@@ -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(


If Count cannot be null then can Count_Big be null?

maumar requested review from AndriySvyryd, roji and bricelam January 15, 2020 23:33

maumar force-pushed the fix18555 branch from 3c88271 to 3b276cb Compare January 16, 2020 00:28

maumar force-pushed the fix18555 branch from 3b276cb to ecdeed4 Compare January 16, 2020 20:40

bricelam reviewed Jan 17, 2020

View reviewed changes

maumar force-pushed the fix18555 branch 4 times, most recently from 9ca9ce7 to 2a8678b Compare January 22, 2020 22:50

bricelam approved these changes Jan 22, 2020

View reviewed changes

maumar force-pushed the fix18555 branch from 2a8678b to d75a0e5 Compare January 22, 2020 22:55

maumar merged commit d75a0e5 into master Jan 22, 2020

maumar deleted the fix18555 branch January 22, 2020 23:52

ajcvickers mentioned this pull request Jan 23, 2020

Entity Framework Weekly Status Updates (2020) #19549

Closed

smitpatel reviewed Feb 6, 2020

View reviewed changes

maumar mentioned this pull request May 28, 2024

Implement nullability computation for SqlFunctionExpression #33814

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to #18555 - Query: when rewriting null semantics for comparisons with functions use function specific metadata to get better SQL #19607

Fix to #18555 - Query: when rewriting null semantics for comparisons with functions use function specific metadata to get better SQL #19607

maumar commented Jan 15, 2020 •

edited

Loading

maumar commented Jan 15, 2020 •

edited

Loading

roji commented Jan 16, 2020

maumar commented Jan 16, 2020

maumar commented Jan 16, 2020

bricelam left a comment

bricelam Jan 22, 2020

smitpatel Feb 6, 2020

maumar Feb 6, 2020

smitpatel Feb 6, 2020

smitpatel Feb 6, 2020

smitpatel Feb 6, 2020

smitpatel Feb 6, 2020

smitpatel Feb 6, 2020

smitpatel Feb 6, 2020

		@@ -485,7 +488,52 @@ public virtual CaseExpression Case(IReadOnlyList<CaseWhenClause> whenClauses, Sq
		}

		public virtual SqlFunctionExpression Function(

Fix to #18555 - Query: when rewriting null semantics for comparisons with functions use function specific metadata to get better SQL #19607

Fix to #18555 - Query: when rewriting null semantics for comparisons with functions use function specific metadata to get better SQL #19607

Conversation

maumar commented Jan 15, 2020 • edited Loading

maumar commented Jan 15, 2020 • edited Loading

roji commented Jan 16, 2020

maumar commented Jan 16, 2020

maumar commented Jan 16, 2020

bricelam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maumar commented Jan 15, 2020 •

edited

Loading

maumar commented Jan 15, 2020 •

edited

Loading