Implementing C# null semantics in the new pipeline #15745
Conversation
src/EFCore.Relational/Query/Pipeline/SqlExpressions/SqlFunctionExpression.cs
Show resolved
Hide resolved
| // !(false) -> true | ||
| if (unaryExpression.NodeType == ExpressionType.Not | ||
| && unaryExpression.Operand is ConstantExpression innerConstant | ||
| && innerConstant.Type == typeof(bool)) |
There was a problem hiding this comment.
innerConstant.Value is bool value
avoids the cast below
src/EFCore.Relational/Query/PipeLine/NullSemanticsRewritingVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/PipeLine/NullSemanticsRewritingVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/PipeLine/NullSemanticsRewritingVisitor.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| var newElseResult = (SqlExpression)Visit(caseExpression.ElseResult); | ||
| _isNullable |= isNullable; |
There was a problem hiding this comment.
Certain expressions like ElseResult could be null too. Is value of _isNullable correct in such cases? Are we not ending up with stale value if we are not resetting the value before each visit.
|
@smitpatel new version up |
| Expression.NotEqual(newArgument, _constantNullString), | ||
| methodCallExpression.Update(newObject, new[] { newArgument }))); | ||
|
|
||
| if (newArgument is ConstantExpression) |
| Expression.NotEqual(newArgument, _constantNullString), | ||
| Expression.Not(innerMethodCall.Update(newObject, new[] { newArgument })))); | ||
|
|
||
| if (newArgument is ConstantExpression) |
test/Directory.Build.props
Outdated
| @@ -2,7 +2,7 @@ | |||
| <Import Project="..\Directory.Build.props" /> | |||
|
|
|||
| <PropertyGroup> | |||
| <StandardTestTfms>netcoreapp3.0</StandardTestTfms> | |||
| <StandardTestTfms>net461;netcoreapp3.0</StandardTestTfms> | |||
There was a problem hiding this comment.
We are deleting this file in Brice's PR. Should revert this change
|
|
||
| Assert.Equal(405, result.Count); | ||
| var result = query.ToList(); | ||
| Assert.Equal(162, result.Count); |
There was a problem hiding this comment.
What is the correct result here?
There was a problem hiding this comment.
debatable, it's for relational null semantics so we cant use L2O as baseline. 162 is assuming null == null doesn't match, 405 is where they match. Seems like 162 should be the expected result.
| } | ||
| } | ||
|
|
||
| public static TResult Maybe<TResult>(object caller, Func<TResult> expression) |
| // !(false) -> true | ||
| if (outerUnary.OperatorType == ExpressionType.Not | ||
| && outerUnary.Operand is SqlConstantExpression innerConstantBool | ||
| && innerConstantBool.Type == typeof(bool)) |
There was a problem hiding this comment.
Value is bool pattern match
| // NULL IS NULL -> true | ||
| if (outerUnary.OperatorType == ExpressionType.Equal | ||
| && outerUnary.Operand is SqlConstantExpression innerConstantNull1 | ||
| && innerConstantNull1.Value == null) |
There was a problem hiding this comment.
If value is not null then you can always return false. Likewise for block below. Basically, you can reduce constant IS NULL check to true/false by looking at the value.
| if (innerUnary.OperatorType == ExpressionType.Equal) | ||
| { | ||
| //!(a IS NULL) -> a IS NOT NULL | ||
| return Visit(_sqlExpressionFactory.IsNotNull(innerUnary.Operand)); |
There was a problem hiding this comment.
nit: Visit only innerUnary.Operand and then create the expression
There was a problem hiding this comment.
we would potentially be missing some optimizations
| //!(a IS NOT NULL) -> a IS NULL | ||
| if (innerUnary.OperatorType == ExpressionType.NotEqual) | ||
| { | ||
| return Visit(_sqlExpressionFactory.IsNull(innerUnary.Operand)); |
There was a problem hiding this comment.
Same as above & other blocks below too
| // true || a -> true | ||
| // false && a -> false | ||
| // false || a -> a | ||
| if (newLeftConstant != null) |
There was a problem hiding this comment.
Remove if above and use else if
|
|
||
| if (sqlBinaryExpression.OperatorType == ExpressionType.Equal) | ||
| { | ||
| if (!leftNullable && !rightNullable) |
There was a problem hiding this comment.
Better represented by nested ifs with 1 condition at time
There was a problem hiding this comment.
Personally for me I think the current grouping is more readable
There was a problem hiding this comment.
The we need to be explicit about the flags in block 3 & 4. It implicitly assumes value of the other flag which is not straight forward to understand.
| // 1 | 0 | 0 | 1 | 0 | | ||
| // 1 | 1 | 1 | 1 | 1 | | ||
| // 1 | N | N | 0 | 0 | | ||
| private SqlBinaryExpression ExpandNonNullableEqualNullable( |
There was a problem hiding this comment.
Just invert the arguments in ExpandNullableEqualNonNullable
There was a problem hiding this comment.
was thinking about it, but didn't want to switch the order of terms in generated sql. I guess its not a big deal tho - will DRY
| switch (extensionExpression) | ||
| { | ||
| case SqlConstantExpression sqlConstantExpression: | ||
| return ProcessSqlConstantExpression(sqlConstantExpression); |
There was a problem hiding this comment.
Maybe just call these Visit* as elsewhere?
src/EFCore.Relational/Query/PipeLine/NullSemanticsRewritingVisitor.cs
Outdated
Show resolved
Hide resolved
|
|
||
| if (sqlBinaryExpression.OperatorType == ExpressionType.Equal) | ||
| { | ||
| if (!leftNullable && !rightNullable) |
There was a problem hiding this comment.
Personally for me I think the current grouping is more readable
| { | ||
| private readonly ISqlExpressionFactory _sqlExpressionFactory; | ||
|
|
||
| private readonly Dictionary<ExpressionType, ExpressionType> _expressionTypesNegationMap |
There was a problem hiding this comment.
C# 8 switch expression FTW (more terse, more efficient):
| private readonly Dictionary<ExpressionType, ExpressionType> _expressionTypesNegationMap | |
| private static ExpressionType Negate(ExpressionType expressionType) | |
| => expressionType switch { | |
| ExpressionType.Equal => ExpressionType.NotEqual, | |
| ExpressionType.NotEqual => ExpressionType.Equal, | |
| ExpressionType.GreaterThan => ExpressionType.LessThanOrEqual, | |
| ExpressionType.GreaterThanOrEqual => ExpressionType.LessThan, | |
| ExpressionType.LessThan => ExpressionType.GreaterThanOrEqual, | |
| ExpressionType.LessThanOrEqual => ExpressionType.GreaterThan | |
| }; |
If you need to check whether negation is possible, we can have TryNegate.
There was a problem hiding this comment.
Which one would be more performant? Readonly dictionary should give O(1) access. Methodcall may need to check through all cases (rejected ones) before reaching correct one.
There was a problem hiding this comment.
A switch over an enum is a switch over its underlying type - so over an int here, which in most cases will do an efficient lookup table under the hood. This is in general much faster than any dictionary - see benchmark below. Also no allocations whatsoever for the dictionary and its internals.
Note that the difference here specifically may be negligible in the grander scheme - especially since this is query compilation only - but a switch expression also seems more terse so why not.
And because it's so easy to do, here's a benchmark comparison:
// * Summary *
BenchmarkDotNet=v0.11.5, OS=ubuntu 19.04
Intel Core i7-6700HQ CPU 2.60GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100-preview4-011223
[Host] : .NET Core 3.0.0-preview5-27626-15 (CoreCLR 4.6.27622.75, CoreFX 4.700.19.22408), 64bit RyuJIT
DefaultJob : .NET Core 3.0.0-preview5-27626-15 (CoreCLR 4.6.27622.75, CoreFX 4.700.19.22408), 64bit RyuJIT
| Method | Mean | Error | StdDev | Median |
|---|---|---|---|---|
| SwitchExpression | 2.553 ns | 0.0884 ns | 0.1150 ns | 2.533 ns |
| Dictionary | 7.055 ns | 0.1993 ns | 0.4775 ns | 6.813 ns |
public class SwitchEnum
{
[Benchmark]
public ExpressionType SwitchExpression()
=> SwitchMethod(ExpressionType.GreaterThanOrEqual);
[Benchmark]
public ExpressionType Dictionary()
=> _dictionary[ExpressionType.GreaterThanOrEqual];
static readonly Dictionary<ExpressionType, ExpressionType> _dictionary
= new Dictionary<ExpressionType, ExpressionType>
{
{ ExpressionType.Equal, ExpressionType.NotEqual },
{ ExpressionType.NotEqual, ExpressionType.Equal },
{ ExpressionType.GreaterThan, ExpressionType.LessThanOrEqual },
{ ExpressionType.GreaterThanOrEqual, ExpressionType.LessThan },
{ ExpressionType.LessThan, ExpressionType.GreaterThanOrEqual },
{ ExpressionType.LessThanOrEqual, ExpressionType.GreaterThan },
};
static ExpressionType SwitchMethod(ExpressionType expressionType)
=> expressionType switch {
ExpressionType.Equal => ExpressionType.NotEqual,
ExpressionType.NotEqual => ExpressionType.Equal,
ExpressionType.GreaterThan => ExpressionType.LessThanOrEqual,
ExpressionType.GreaterThanOrEqual => ExpressionType.LessThan,
ExpressionType.LessThan => ExpressionType.GreaterThanOrEqual,
ExpressionType.LessThanOrEqual => ExpressionType.GreaterThan,
_ => throw new ArgumentException(nameof(expressionType))
};
}There was a problem hiding this comment.
If switch is faster let's use it. Is it specific for Enum case only of any type? I would guess not all pattern matching is as cheap as enums over int.
There was a problem hiding this comment.
Here are the results for string keys, switch wins as well:
| Method | Mean | Error | StdDev |
|---|---|---|---|
| SwitchExpression | 13.29 ns | 0.2945 ns | 0.4317 ns |
| Dictionary | 23.10 ns | 0.4976 ns | 1.0387 ns |
In general, for small datasets switch should (almost) always be faster than a dictionary - when in doubt we can always do a quick benchmark to find out.
| } | ||
| } | ||
|
|
||
| if (extensionExpression is SqlBinaryExpression outerBinary) |
There was a problem hiding this comment.
This method already deals with a lot of cases, maybe split to one method for (outer) unary and a separate one for (outer) binary?
There was a problem hiding this comment.
the reason why I didnt split them is so I can easily fall back to base.VisitExtension call. But I guess I can just return null in the VisitUnary/VisitBinary for unoptimized cases and test for that in the VisitExtension
There was a problem hiding this comment.
Just a proposal, no strong feelings about it... :)
There was a problem hiding this comment.
i think its a good suggestion, especially when/if we add more optimizations, the single method will become unreadable
src/EFCore.SqlServer/Query/Pipeline/SearchConditionConvertingExpressionVisitor.cs
Show resolved
Hide resolved
| { | ||
| public class NegationOptimizingVisitor : ExpressionVisitor | ||
| { | ||
| private readonly Dictionary<ExpressionType, ExpressionType> _expressionTypesNegationMap |
There was a problem hiding this comment.
Same as above, C# 8 switch expression
|
new version up @smitpatel @roji |
| // however if we decide to do "partial" null semantics (that doesn't distinguish between NULL and FALSE, e.g. for predicates) | ||
| // we need to be extra careful here | ||
| var reverse = ReverseExpressionType(innerBinary.OperatorType); | ||
| if (reverse != innerBinary.OperatorType) |
There was a problem hiding this comment.
this seems a bit awkward. Any better way to replace ContainsKey() check? @roji
There was a problem hiding this comment.
thinking to revert it back to dictionary for now just to get the feature in - will refactor into switch in future checkin if we come up with a nice pattern for the ContainsKey thing
There was a problem hiding this comment.
I think your force-push made some commits disappear, can't see this exact code.
In any case, I'd have a TryReverse() which returns bool (like Dictionary.TryGetValue()), upon which you could implement Reverse() (which throws if not found, like Dictionary.this[]).
There was a problem hiding this comment.
(sorry, had a weird unrefreshed browser version where this PR wasn't merged yet)
| return newArgument is ConstantExpression | ||
| ? result | ||
| : Expression.OrElse( | ||
| Expression.Equal( |
| Expression.Not(innerBinary.Right))); | ||
| } | ||
|
|
||
| var reverse = ReverseExpressionType(innerBinary.NodeType); |
There was a problem hiding this comment.
return null and use null check
There was a problem hiding this comment.
There was a problem hiding this comment.
but then I need to use .Value everywhere - seems even more awkward
| @@ -8,6 +8,7 @@ | |||
| <RootNamespace>Microsoft.EntityFrameworkCore</RootNamespace> | |||
| <GenerateDocumentationFile>true</GenerateDocumentationFile> | |||
| <CodeAnalysisRuleSet>..\..\EFCore.ruleset</CodeAnalysisRuleSet> | |||
| <LangVersion>8.0</LangVersion> | |||
There was a problem hiding this comment.
was added by VS when i used c# 8.0 "switch" - will remove
There was a problem hiding this comment.
If we're OK with switch expressions, that's new to C# 8, as opposed to plain old verbose switch statements. Although for now I don't see that change.
There was a problem hiding this comment.
(sorry, had a weird unrefreshed browser version where this PR wasn't merged yet)
| @@ -49,7 +49,7 @@ var results | |||
| Assert.Equal( | |||
| @"SELECT [m].[Int] | |||
| FROM [MappedNullableDataTypes] AS [m] | |||
| WHERE [m].[TimeSpanAsTime] = '00:01:02'", | |||
| WHERE ([m].[TimeSpanAsTime] = '00:01:02') AND [m].[TimeSpanAsTime] IS NOT NULL", | |||
| // non_nullablee_constant IS NOT NULL -> true | ||
| if (sqlUnaryExpression.OperatorType == ExpressionType.NotEqual | ||
| && sqlUnaryExpression.Operand is SqlConstantExpression innerConstantNull2 | ||
| && innerConstantNull2.Value == null) |
| return sqlBinaryExpression.Update(newLeft, newRight); | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
Can we just do visit here rather than calling base in initial method?
Currently we always apply "full" null semantics translation, meaning null values are completely removed from the tree. We could produce slightly simpler translation where it doesn't matter if we return NULL or FALSE (e.g. in predicates) Also, currently when testing if a given subtree is null we do it in naive way, simply adding IsNull call around entire subtree. Instead we can test it's constituents, but to do it properly we need to persist nullability information.
ghost
deleted the
Currently we always apply "full" null semantics translation, meaning null values are completely removed from the tree. We could produce slightly simpler translation where it doesn't matter if we return NULL or FALSE (e.g. in predicates)
Also, currently when testing if a given subtree is null we do it in naive way, simply adding IsNull call around entire subtree. Instead we can test it's constituents, but to do it properly we need to persist nullability information.