Support of ExclusiveOr, RightShift, LeftShift

[dmitry-lipetsk]

Hello,

I want create a support for operator ^ (ExclusiveOr). I will map it to Firebird builtin function BIN_XOR.

But SqlBinaryExpression does not support it:

efcore/src/EFCore.Relational/Query/SqlExpressions/SqlBinaryExpression.cs

Lines 28 to 50 in e260690

	private static readonly ISet<ExpressionType> _allowedOperators = new HashSet<ExpressionType>
	{
	ExpressionType.Add,
	ExpressionType.Subtract,
	ExpressionType.Multiply,
	ExpressionType.Divide,
	ExpressionType.Modulo,
	//ExpressionType.Power,
	ExpressionType.And,
	ExpressionType.AndAlso,
	ExpressionType.Or,
	ExpressionType.OrElse,
	ExpressionType.LessThan,
	ExpressionType.LessThanOrEqual,
	ExpressionType.GreaterThan,
	ExpressionType.GreaterThanOrEqual,
	ExpressionType.Equal,
	ExpressionType.NotEqual,
	//ExpressionType.ExclusiveOr,
	//ExpressionType.ArrayIndex,
	//ExpressionType.RightShift,
	//ExpressionType.LeftShift,
	};

Same problem, as I understand, with operators << and >>.

---

From other side, EFCore supports '&' and '|' :(

I think, I can map these operators (^, >>, <<) to SqlFunctionExpression nodes.

But this variant looks very ugly.

Your ideas?

[smitpatel]

Please add you custom binary expression like https://github.com/npgsql/efcore.pg/blob/main/src/EFCore.PG/Query/Expressions/Internal/PostgresBinaryExpression.cs

Relational layer is common subset of whatever is supported by most of the database, hence those operators are not in SqlBinaryExpression.

[dmitry-lipetsk]

Ok. Thanks.

[smitpatel]

You can derive from SqlBinaryExpression or derive from SqlExpression. The choice would depend on the processing of SqlBinary in other places (like SqlNullabilityProcessor or optimizing visitors etc). SqlExpressionFactory.MakeBinary would allow you to create your custom SqlBinaryExpression if you derive from SqlBinary as base would return null if it is not a supported operator.

[dmitry-lipetsk]

Hello,

1. Derive from SqlExpression - it is bad variant. I want to use your SqlNullabilityProcessor optimizer. Me like it :)

2. Seem I may derive your SqlBinaryExpression for "hack" it. But I see another small problem. In SQL I translate SqlBinaryExpression to "function call". For example "a&b" to "BIN_AND(a,b)".

Your QuerySqlGenerator wrap SqlBinaryExpression into round brackets. As result I get "(BIN_AND(a,b))" instead simple "BIN_AND(a,b)"

So, seem the better to translate these binary operators (&, |, ^, >>, <<) to SqlFunctionExpression. This will resolve all my problems.

For this I should override your RelationalSqlTranslatingExpressionVisitor::VisitBinary, copy it code to my method and append "selector" for different ExpressionTypes.

Why do not use in RelationalSqlTranslatingExpressionVisitor::VisitBinary external translator IBinaryOperatorTranslatorProvider, as IMethodCallTranslatorProvider in VisitMethodCall?

You implement this IBinaryOperatorTranslatorProvider with your default behavior.

I implement in it my translators.

What do you think about this idea?

[roji]

Derive from SqlExpression - it is bad variant. I want to use your SqlNullabilityProcessor optimizer. Me like it :)

Why do you see this as a bad variant? You can easily use SqlNullabilityProcessor when doing this - see the EFCore.PG example.

In SQL I translate SqlBinaryExpression to "function call". For example "a&b" to "BIN_AND(a,b)". [...] So, seem the better to translate these binary operators (&, |, ^, >>, <<) to SqlFunctionExpression. This will resolve all my problems.

Indeed, if what you're doing is translating to a SQL function in the end, you should probably not be passing through any sort of SqlBinaryExpression during translation. In other words, consider just translating the original expression tree's BinaryExpression (with the bitwise operation) directly to an SqlFunctionExpression (BIN_AND). A custom SqlExpression is needed mostly when you want to actually output a new type of binary expression in SQL (as opposed to a function call).

For this I should override your RelationalSqlTranslatingExpressionVisitor::VisitBinary, copy it code to my method and append "selector" for different ExpressionTypes.

No need to copy any code - you can simply override VisitBinary, check for your special case and handle it (the bitwise operators), and call base.VisitBinary otherwise. See this for an example.

Why do not use in RelationalSqlTranslatingExpressionVisitor::VisitBinary external translator IBinaryOperatorTranslatorProvider, as IMethodCallTranslatorProvider in VisitMethodCall?

It's certainly possible, but there has been very little need for binary expression extensibility - much less than functions and members where the extensibility is potentially infinite. It's really fine to implement functionality for binary expressions directly in your override RelationalSqlTranslatingExpressionVisitor.VisitBinary.

[dmitry-lipetsk]

Derive from SqlExpression - it is bad variant. I want to use your SqlNullabilityProcessor optimizer. Me like it :)

Why do you see this as a bad variant? You can easily use SqlNullabilityProcessor when doing this - see the EFCore.PG example.

Oh sh... Again create subclass ... :)

No need to copy any code - you can simply override VisitBinary, check for your special case and handle it (the bitwise operators), and call base.VisitBinary otherwise. See this for an example.

I want just replace call of _sqlExpressionFactory.MakeBinary to call of my method.

It would be nice if for this didn't require replacing the entire method VisitBinary....

Why do not use in RelationalSqlTranslatingExpressionVisitor::VisitBinary external translator IBinaryOperatorTranslatorProvider, as IMethodCallTranslatorProvider in VisitMethodCall?

It's certainly possible, but there has been very little need for binary expression extensibility - much less than functions and members where the extensibility is potentially infinite. It's really fine to implement functionality for binary expressions directly in your override RelationalSqlTranslatingExpressionVisitor.VisitBinary.

Already done in my code :)

[smitpatel]

• If you are writing a provider, you have to add subclasses if you want to do anything beyond what Relational provides. I don't thing there is anything inherently wrong with subclasses.
• You can also call base implementation from a derived method.

[dmitry-lipetsk]

@smitpatel I prefer to inherited the interface or override abstract methods. Current design of EFCore, where I should override methods creates a some problem for me.

But ok.

I implemented my BinaryOperationTranslationProvider.

It help to generate correct SQL and allow create more clear code. For example, my QuerySqlGenerator do not required code like this:

https://github.com/cincuranet/FirebirdSql.Data.FirebirdClient/blob/7607beb35e9eec693675d2828daf600f7d2a6d44/Provider/src/FirebirdSql.EntityFrameworkCore.Firebird/Query/Internal/FbQuerySqlGenerator.cs#L43-L96

https://github.com/cincuranet/FirebirdSql.Data.FirebirdClient/blob/7607beb35e9eec693675d2828daf600f7d2a6d44/Provider/src/FirebirdSql.EntityFrameworkCore.Firebird/Query/Internal/FbQuerySqlGenerator.cs#L186-L193

But, seems, I have a real problem.

I want to use my BinaryOperationTranslationProvider instead your final code in VisitBinary:

efcore/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs

Lines 356 to 362 in e260690

	: uncheckedNodeTypeVariant == ExpressionType.Coalesce
	? _sqlExpressionFactory.Coalesce(sqlLeft!, sqlRight!)
	: _sqlExpressionFactory.MakeBinary(
	uncheckedNodeTypeVariant,
	sqlLeft!,
	sqlRight!,
	null) ?? QueryCompilationContext.NotTranslatedExpression;

For this, I should copy your implementation of VisitBinary (and code of all methods that called from VisitBinary) and replace these last lines.

Unfortunately, I can't make this, because your TryRewriteEntityEquality uses private type - EntityReferenceExpression.

First variant
Please define TryRewriteEntityEquality as protected.

For me, at current time, it is preferred solution.

Second variant
Please move this code:

uncheckedNodeTypeVariant == ExpressionType.Coalesce
                        ? _sqlExpressionFactory.Coalesce(sqlLeft!, sqlRight!)
                        : _sqlExpressionFactory.MakeBinary(
                            uncheckedNodeTypeVariant,
                            sqlLeft!,
                            sqlRight!,
                            null)

into new protected virtual method, that return SqlExpression. I will override this method in my code.

---

Or implement both variants :)

---

My implementation of VisitBinary:

 protected override Expression VisitBinary(BinaryExpression binaryExpression)
 {
  Check.Arg_NotNull
   (c_ErrSrcID,
    nameof(VisitBinary),
    nameof(binaryExpression),
    binaryExpression);

  //----------------------------------------------------------------------
  Debug.Assert(!Object.ReferenceEquals(binaryExpression.Left,null));
  Debug.Assert(!Object.ReferenceEquals(binaryExpression.Right,null));

  //----------------------------------------------------------------------
  if(binaryExpression.NodeType==ExpressionType.Equal)
  {
   if(binaryExpression.Left is NewArrayExpression)
   {
    if(binaryExpression.Left.Type == Structure_TypeCache.TypeOf__System_Array_Object)
    {
     var x
      =Helper__EFCore__ConvertObjectArrayEqualityComparison
        (binaryExpression.Left,
         binaryExpression.Right);

     Debug.Assert(!Object.ReferenceEquals(x,null));

     return this.Visit(x);
    }//if
   }//if
  }//if

  var left  = Helper__EFCore__TryRemoveImplicitConvert(binaryExpression.Left);
  var right = Helper__EFCore__TryRemoveImplicitConvert(binaryExpression.Right);

  // Remove convert-to-object nodes if both sides have them, or if the other side is null constant
  var isLeftConvertToObject  = Helper__EFCore__TryUnwrapConvertToObject(left,out var leftOperand);
  var isRightConvertToObject = Helper__EFCore__TryUnwrapConvertToObject(right,out var rightOperand);

  if(isLeftConvertToObject && isRightConvertToObject)
  {
   left  = leftOperand;
   right = rightOperand;
  }
  else
  if(isLeftConvertToObject && right.Extension__IsNullConstantExpression())
  {
   left=leftOperand;
  }
  else
  if(isRightConvertToObject && left.Extension__IsNullConstantExpression())
  {
   right=rightOperand;
  }//if

  var visitedLeft  = this.Visit(left);
  var visitedRight = this.Visit(right);

  if(binaryExpression.NodeType == ExpressionType.Equal ||
     binaryExpression.NodeType == ExpressionType.NotEqual)
  {
   // Visited expression could be null, We need to pass MemberInitExpression
   if(base.TryRewriteEntityEquality
       (binaryExpression.NodeType,
        visitedLeft  == QueryCompilationContext.NotTranslatedExpression ? left  : visitedLeft,
        visitedRight == QueryCompilationContext.NotTranslatedExpression ? right : visitedRight,
        equalsMethod: false,
        out var result))
   {
    return result;
   }//if
  }//if

  if(Helper__EFCore__TranslationFailed(binaryExpression.Left,visitedLeft,out var sqlLeft))
   return QueryCompilationContext.NotTranslatedExpression;

  if(Helper__EFCore__TranslationFailed(binaryExpression.Right,visitedRight,out var sqlRight))
   return QueryCompilationContext.NotTranslatedExpression;

  ExpressionType
   uncheckedNodeTypeVariant
    =Helper__EFCore__GetUncheckedNodeTypeVariant(binaryExpression.NodeType);

  Debug.Assert(!Object.ReferenceEquals(this.Dependencies,null));
  Debug.Assert(!Object.ReferenceEquals(this.Dependencies.SqlExpressionFactory,null));

  if(uncheckedNodeTypeVariant==ExpressionType.Coalesce)
  {
   //! \todo Move into m_BinaryOperatorTranslatorProvider

   return Dependencies.SqlExpressionFactory.Coalesce(sqlLeft,sqlRight);
  }//if

  Debug.Assert(!Object.ReferenceEquals(m_BinaryOperatorTranslatorProvider,null));

  Debug.Assert(!Object.ReferenceEquals(m_QueryCompilationContext,null));

  var r=m_BinaryOperatorTranslatorProvider.Translate
         (m_QueryCompilationContext.Model,
          uncheckedNodeTypeVariant,
          sqlLeft,
          sqlRight,
          binaryExpression.Type,
          m_QueryCompilationContext.Logger);

  if(Object.ReferenceEquals(r,null))
  {
   return QueryCompilationContext.NotTranslatedExpression;
  }//if

  return r;
 }//VisitBinary

[smitpatel]

Call base.VisitBinary - I don't see any value in making any additional methods protected. We provide extensibility based on ease and merits. Your preference in writing code is irrelevant to me.

[ajcvickers]

@dmitry-lipetsk While we certainly invite constructive feedback on how to improve the extensibility model for providers, I think there is a point when where both sides have to accept that they are not going to see things the same way. I don't think it's very productive for you to be repeatedly filing issues suggesting that we make the same kinds of changes when we don't believe these changes add value, or even fit into the extensibility model that we have.

Also, it seems apparent from the changes you suggest that you don't yet have a deep understanding of the full scope of database provider interactions. I think you would be much better off focusing your time on getting your database provider up and running with all the specification tests passing. At that point it will perhaps be more apparent why the extensibility model is what it is.

I'm not trying to be mean or discouraging here. However, we get a lot of issues filed and work with many people submitting PRs and building providers, and we have to be realistic how much time we can continue to put into responding to the same kinds of issue from one person.

[dmitry-lipetsk]

Yor final code

efcore/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs

Lines 356 to 362 in e260690

	: uncheckedNodeTypeVariant == ExpressionType.Coalesce
	? _sqlExpressionFactory.Coalesce(sqlLeft!, sqlRight!)
	: _sqlExpressionFactory.MakeBinary(
	uncheckedNodeTypeVariant,
	sqlLeft!,
	sqlRight!,
	null) ?? QueryCompilationContext.NotTranslatedExpression;

may return

• SqlFunctionExpression from _sqlExpressionFactory.Coalesce
• SqlBinaryExpression from _sqlExpressionFactory.MakeBinary

Just append the way for return SqlExpression (in this point) from another (my) translator.