Efcore: SqlBinaryExpression should be sealed

[MaxG117]

SqlBinaryExpression (and possibly SqlUnaryExpression) should be sealed. I have a custom expression, which in 2.2 was derived from BinaryExpression. So in 3.1 I derived it from SqlBinaryExpression instead, which seemed more appropriate. The instance of my derived class gets replaced by an instance of SqlBinaryExpression without any warning in SqlExpressionOptimizingExpressionVisitor.VisitSqlBinaryExpression:

``` C#
return SimplifyBinaryExpression(
sqlBinaryExpression.OperatorType,
newLeft,
newRight,
sqlBinaryExpression.TypeMapping);

because of 
``` C#
        protected override Expression VisitExtension(Expression extensionExpression)
            => extensionExpression switch
            {
                SqlUnaryExpression sqlUnaryExpression => VisitSqlUnaryExpression(sqlUnaryExpression),
                SqlBinaryExpression sqlBinaryExpression => VisitSqlBinaryExpression(sqlBinaryExpression),
                SelectExpression selectExpression => VisitSelectExpression(selectExpression),
                _ => base.VisitExtension(extensionExpression),
            };

If custom expressions cannot be derived from SqlBinaryExpression (or SqlUnaryExpression, or SelectExpression), then it should be sealed.

Further technical details

EF Core version: 3.1.4
Database provider: the one I'm writing

[smitpatel]

If you want to change anything in Sql tree, you need to also implement ISqlExpressionFactory which creates your custom derived expression rather than base one.

[MaxG117]

I did not have to implement ISqlExpressionFactory, the default implementation works fine. I implemented:
1) A custom expression visitor derived from QuerySqlGenerator and instantiated with my implementation of IQuerySqlGeneratorFactory.
2) A custom expression whose Accept method calls the appropriate Visit*Expression method in my expression visitor.

Specifically, I implemented the String.Compare(string strA, string strB, bool ignoreCase) method, which I don't think any other providers implemented. The other String.Compare methods translate to a SqlBinaryExpression, so I initially created my own StringCompareExpression : SqlBinaryExpression with a field added for ignoreCase. The method translation worked, but then my StringCompareExpression was replaced by SqlBinaryExpression as mentioned in the first comment. So my Visit*Expression was never called, because my custom expression no longer existed. The solution was to derive my StringCompareExpression from SqlExpression instead of SqlBinaryExpression to prevent this automatic replacement.

My conclusion is that no expression could or should be derived from SqlBinaryExpression. To prevent others from repeating my mistake, SqlBinaryExpression must be sealed. I don't see how ISqlExpressionFactory is related to this.

[smitpatel]

My conclusion is that no expression could or should be derived from SqlBinaryExpression. To prevent others from repeating my mistake, SqlBinaryExpression must be sealed. I don't see how ISqlExpressionFactory is related to this.

• All the visitors/translators utilize ISqlExpressionFactory to construct SQL tree. Anything custom SQL expression being created outside of that may not work in all cases as every visitor is going to use SqlExpressionFactory and will never create the custom SQL expression.
• The expression you were trying to add is not binary expression as it has 3 operands.
• A derived SqlBinaryExpression can support more types of binary operation than what base supports. Provider can override ISqlExpressionFactory.MakeBinary method to produce the derived SqlBinaryExpression when operator type is one of those which is not understood by the base one.

[roji]

@MaxG117 I'm also curious why you'd want to represent string.Compare via a custom expression type, rather than just leaving it an SqlFunctionExpression as it is by default - that seems to be making something more complex than it has to be. If you add some more context on what you're trying to do we may be able to point out an easier direction.

[MaxG117]

@roji SqlFunctionExpressions are translated by RelationalMethodCallTranslatorProvider. Other String.Compare methods are translated by the ComparisonTranslator into a CaseExpression with three CaseWhenClauses that compare a SqlBinaryExpression with 0, 1, and -1. I was following the same pattern, with the exception that my CaseWhenClauses contain my custom expression type, which retains the value of ignoreCase.

[roji]

@MaxG117 I don't have enough context on what you're doing or what you're trying to translate into... If your database has a special non-standard SQL construct for a case-insensitive/sensitive comparison, then a custom expression type could be the right thing. However, if in the end you're emitting standard SQL, then chances are you should not be creating a custom expression. You should be able to write a translator which identifies what you want (comparison with ignoreCare?) and renders whatever standard SQL you want, with or without CaseExpression.

[ajcvickers]

@MaxG117 We discussed this, and we don't believe that the type should be sealed, so I'm closing this as by-design. However, we will be happy to continue helping you if needed.

[MaxG117]

@roji last I checked there is no standard SQL construct for a case-insensitive/sensitive comparison. SqlServer has COLLATE, Teradata has a "(casespecific)"/"(not casespecific)" qualifier added after the string. So while it's no longer a binary operator since it has a third parameter, it could be derived from a binary operator. In version 2.2 my implementation was derived from BinaryExpression.

@smitpatel:

A derived SqlBinaryExpression can support more types of binary operation than what base supports. Provider can override ISqlExpressionFactory.MakeBinary method to produce the derived SqlBinaryExpression when operator type is one of those which is not understood by the base one.

In my case the operator type IS understood by the base implementation. And there was no warning or indication that I'd have to override ISqlExpressionFactory.

Here's the problem:

If Duck is derived from Bird, then a Duck is a Bird. But a Bird is not a Duck.

Your implementation of SqlExpressionOptimizingExpressionVisitor.VisitSqlBinaryExpression destroys the subtype -- it checks whether a Duck is a Bird and then replaces the Duck with a Bird.

If sealing the Bird is not the appropriate solution, then maybe Bird should have an abstract Clone method to support what you are trying to do without destroying the subtype.

[smitpatel]

In my case the operator type IS understood by the base implementation. And there was no warning or indication that I'd have to override ISqlExpressionFactory.

That is incorrect. Where did you pass the case sensitivity flag to the binary operator?

Further if Duck derived from bird but bird is not a duck then you need to do something so new babies are born as duck specifically when required and not as bird.

[MaxG117]

I think you are arguing that my derived operator is no longer a binary operator. I am arguing that I should be able to derive my non-binary operator from your unsealed binary operator. It doesn't matter that I have a third parameter now. I taught my Bird to quack, so now it's a Duck. It still has two wings (Left and Right) and a tail (OperatorType). You are telling me that my Duck is not allowed to have the same tail as a Bird. But that's not explicitly prohibited anywhere, which is why I initially suggested to make the Bird class sealed.

if Duck derived from bird but bird is not a duck then you need to do something so new babies are born as duck specifically when required and not as bird.

If Duck is a Bird, then SqlExpressionOptimizingExpressionVisitor.VisitExtension calls VisitSqlBinaryExpression, which destructively replaces my Duck with your Bird. Your code is replacing my Duck with your Bird. You did not provide me with any way to avoid this. That's why I suggested an abstract Clone method may be needed to ensure that instead of destroying my Duck, your code should invoke bird.Clone(operatorType,left,right) and let my Duck keep its ability to quack.

Regarding your earlier suggestion to override ISqlExpressionFactory.MakeBinary -- it does not make sense in this case, because the parameters for MakeBinary are members of the SqlBinaryExpression (operatorType, left, and right). By the time we get to call MakeBinary, the Duck's ability to quack has already been lost. The only way for me to fix this would be to override SqlExpressionOptimizingExpressionVisitor.VisitExtension or VisitSqlBinaryExpression and remove the assumption that nothing can be derived from SqlBinaryExpression.

[MaxG117]

@ajcvickers Your implementation of SqlExpressionOptimizingExpressionVisitor is performing a destructive replacement of a sublcass with a superclass. My initial suggestion was to make the superclasss sealed, which would disallow any subclasses and avoid the problem. Another solution is to fix the replacement so that it is not destructive, as I tried to explain above. Please discuss this one more time.

I spent two days figuring out why my subclass was disappearing from the expression tree and my team feels strongly that this is a problem in your logic.

[smitpatel]

@MaxG117 - Here is the overall design of the expressions in SQL tree,

• SqlExpressionFactory is source of truth and it is used to create most expression types except for a few which are already sealed. Anyone rather be it be EF Core or provider writers or plug in writers should be using SqlExpressionFactory to create SQL expressions.
• The unsealed classes are free to override but not change fundamental meaning of the expression. A binary operation takes 2 arguments and 1 operator. Just because you can derive in C# to add additional properties to serve as argument does not mean that it is allowed in SQL tree because it is not longer a binary operation. If there was a way to put this restrictions in C# then we would have done but there is no way to tell C# that your hierarchy is much more flexible than SQL tree hierarchy and don't do it.
• If a provider wants to add additional operation based on Sql Binary which are true binary or want to change how equality is implemented for certain operations, a non-sealed class still allows them to do it in conjunction with overriding SqlExpressionFactory.MakeBinary method which will generate derived expression for provider and that will work end to end without any destruction.

Your argument is that we should make it sealed because it helps your case. According to us, the way you are using it is incorrect and just to catch the incorrect usage, restricting ability for correct usage is wrong. We are willing to make changes to our expressions for correct usage scenario if it is blocking in anyway. Your expression is not binary operation, use your own custom expression. You can always use your custom expressions even if it matches structure of our expression if you want. But if you decide to use our expression then you have to override SqlExpressionFactory.

Writing an EF Core provider especially which dives into the area of query is not an easy task, I suggest you try to understand the query pipeline on holistic level and how each piece interact with each other, before attempting to make changes in query pipeline for your provider. It is a steep learning curve.

[MaxG117]

If there was a way to put this restrictions in C# then we would have done but there is no way to tell C# that your hierarchy is much more flexible than SQL tree hierarchy

Your argument is that we should make it sealed because it helps your case. According to us, the way you are using it is incorrect and just to catch the incorrect usage, restricting ability for correct usage is wrong.

I agree with this, which is why I proposed another solution -- an abstract Clone method.

Just because you can derive in C# to add additional properties to serve as argument does not mean that it is allowed in SQL tree

This seems to be an unwritten rule that I couldn't have known if I did not run into this problem.

Anyway, it's too late for me, I already figured out that deriving my class from SqlExpression instead of SqlBinaryExpression works as intended. I was just trying to save the next developer working on another database provider from making the same mistake I did. I appreciate all of your responses and time.