Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Query: Introduce method for providing brackets around SqlExpression based on operator precedence #26660

Merged
1 commit merged into from
Nov 17, 2021
Merged

Query: Introduce method for providing brackets around SqlExpression based on operator precedence #26660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 62 additions & 27 deletions src/EFCore.Relational/Query/QuerySqlGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ protected override Expression VisitTable(TableExpression tableExpression)
private void GenerateFromSql(FromSqlExpression fromSqlExpression)
{
var sql = fromSqlExpression.Sql;
string[]? substitutions = null;
string[]? substitutions;

switch (fromSqlExpression.Arguments)
{
Expand Down Expand Up @@ -430,7 +430,7 @@ protected virtual void CheckComposableSql(string sql)
{
var i = span.IndexOf('\n');
span = i > 0
? span.Slice(i + 1).TrimStart()
? span[(i + 1)..].TrimStart()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like ranges, but in this very particular case I find it less readable...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why though? How is it different from any other range operation?

Copy link
Member

@roji roji Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just compared to span.Slice(i+1). Less parentheses/brackets, less operators, less clutter.... Not very important.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lesser characters though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷

: throw new InvalidOperationException(RelationalStrings.FromSqlNonComposable);
continue;
}
Expand All @@ -440,7 +440,7 @@ protected virtual void CheckComposableSql(string sql)
{
var i = span.IndexOf("*/");
span = i > 0
? span.Slice(i + 2).TrimStart()
? span[(i + 2)..].TrimStart()
: throw new InvalidOperationException(RelationalStrings.FromSqlNonComposable);
continue;
}
Expand All @@ -459,18 +459,11 @@ protected virtual void CheckComposableSql(string sql)
/// <exception cref="InvalidOperationException">The given SQL isn't composable.</exception>
protected virtual void CheckComposableSqlTrimmed(ReadOnlySpan<char> sql)
{
if (sql.StartsWith("SELECT", StringComparison.OrdinalIgnoreCase))
{
sql = sql.Slice("SELECT".Length);
}
else if (sql.StartsWith("WITH", StringComparison.OrdinalIgnoreCase))
{
sql = sql.Slice("WITH".Length);
}
else
{
throw new InvalidOperationException(RelationalStrings.FromSqlNonComposable);
}
sql = sql.StartsWith("SELECT", StringComparison.OrdinalIgnoreCase)
? sql["SELECT".Length..]
smitpatel marked this conversation as resolved.
Show resolved Hide resolved
: sql.StartsWith("WITH", StringComparison.OrdinalIgnoreCase)
? sql["WITH".Length..]
: throw new InvalidOperationException(RelationalStrings.FromSqlNonComposable);

if (sql.Length > 0
&& (char.IsWhiteSpace(sql[0]) || sql.StartsWith("--") || sql.StartsWith("/*")))
Expand All @@ -484,7 +477,7 @@ protected virtual void CheckComposableSqlTrimmed(ReadOnlySpan<char> sql)
/// <inheritdoc />
protected override Expression VisitSqlBinary(SqlBinaryExpression sqlBinaryExpression)
{
var requiresBrackets = RequiresBrackets(sqlBinaryExpression.Left);
var requiresBrackets = RequiresParentheses(sqlBinaryExpression, sqlBinaryExpression.Left);

if (requiresBrackets)
{
Expand All @@ -500,7 +493,7 @@ protected override Expression VisitSqlBinary(SqlBinaryExpression sqlBinaryExpres

_relationalCommandBuilder.Append(GetOperator(sqlBinaryExpression));

requiresBrackets = RequiresBrackets(sqlBinaryExpression.Right);
requiresBrackets = RequiresParentheses(sqlBinaryExpression, sqlBinaryExpression.Right);

if (requiresBrackets)
{
Expand All @@ -517,14 +510,6 @@ protected override Expression VisitSqlBinary(SqlBinaryExpression sqlBinaryExpres
return sqlBinaryExpression;
}

private static bool RequiresBrackets(SqlExpression expression)
=> expression is SqlBinaryExpression
|| expression is LikeExpression
|| (expression is SqlUnaryExpression unary
&& unary.Operand.Type == typeof(bool)
&& (unary.OperatorType == ExpressionType.Equal
|| unary.OperatorType == ExpressionType.NotEqual));

/// <inheritdoc />
protected override Expression VisitSqlConstant(SqlConstantExpression sqlConstantExpression)
{
Expand Down Expand Up @@ -661,7 +646,7 @@ protected override Expression VisitSqlUnary(SqlUnaryExpression sqlUnaryExpressio
case ExpressionType.Convert:
{
_relationalCommandBuilder.Append("CAST(");
var requiresBrackets = RequiresBrackets(sqlUnaryExpression.Operand);
var requiresBrackets = RequiresParentheses(sqlUnaryExpression, sqlUnaryExpression.Operand);
if (requiresBrackets)
{
_relationalCommandBuilder.Append("(");
Expand Down Expand Up @@ -712,7 +697,7 @@ protected override Expression VisitSqlUnary(SqlUnaryExpression sqlUnaryExpressio
case ExpressionType.Negate:
{
_relationalCommandBuilder.Append("-");
var requiresBrackets = RequiresBrackets(sqlUnaryExpression.Operand);
var requiresBrackets = RequiresParentheses(sqlUnaryExpression, sqlUnaryExpression.Operand);
if (requiresBrackets)
{
_relationalCommandBuilder.Append("(");
Expand Down Expand Up @@ -790,6 +775,56 @@ protected override Expression VisitIn(InExpression inExpression)
protected virtual string GetOperator(SqlBinaryExpression binaryExpression)
=> _operatorMap[binaryExpression.OperatorType];

/// <summary>
/// Returns a bool value indicating if the inner SQL expression required to be put inside parenthesis
/// when generating SQL for outer SQL expression.
/// </summary>
/// <param name="outerExpression">The outer expression which provides context in which SQL is being generated.</param>
/// <param name="innerExpression">The inner expression which may need to be put inside parenthesis.</param>
/// <returns>A bool value indicating that parenthesis is required or not. </returns>
protected virtual bool RequiresParentheses(SqlExpression outerExpression, SqlExpression innerExpression)
{
switch (innerExpression)
smitpatel marked this conversation as resolved.
Show resolved Hide resolved
{
case LikeExpression _:
return true;

case SqlUnaryExpression sqlUnaryExpression:
{
// Wrap IS (NOT) NULL operation when applied on bool column.
if ((sqlUnaryExpression.OperatorType == ExpressionType.Equal
|| sqlUnaryExpression.OperatorType == ExpressionType.NotEqual)
&& sqlUnaryExpression.Operand.Type == typeof(bool))
{
return true;
}

return false;
}

case SqlBinaryExpression sqlBinaryExpression:
{
//if (outerExpression is SqlBinaryExpression outerBinary)
//{
// if (outerBinary.OperatorType == ExpressionType.AndAlso)
// {
// return sqlBinaryExpression.OperatorType == ExpressionType.OrElse;
// }

// if (outerBinary.OperatorType == ExpressionType.OrElse)
// {
// // Precedence-wise AND is above OR but we still add parenthesis for ease of understanding
// return sqlBinaryExpression.OperatorType == ExpressionType.AndAlso;
// }
//}

return true;
}
}

return false;
}

/// <summary>
/// Generates a TOP construct in the relational command
/// </summary>
Expand Down
Loading