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.

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

SQL Server: Use XOR to translate some NOT expressions #34080

Merged
merged 3 commits into from
Jun 28, 2024

Conversation

ranma42
Copy link
Contributor

@ranma42 ranma42 commented Jun 24, 2024

When neither the parent expression nor the inner one is a predicate, translate to:

x ^ CAST(1 AS bit)

instead of

CASE
    WHEN x = CAST(0 AS bit) THEN CAST(1 AS bit)
    ELSE CAST(0 AS bit)
END

Contributes to #34001 for simple cases (NOT of BIT expressions).

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

That's a really nice idea @ranma42!

@@ -374,6 +374,18 @@ protected override Expression VisitSqlUnary(SqlUnaryExpression sqlUnaryExpressio
case ExpressionType.Not
when sqlUnaryExpression.Type == typeof(bool):
{
// when possible, avoid converting to/from predicate form
if (!_isSearchCondition && !(sqlUnaryExpression.Operand is ExistsExpression or InExpression or LikeExpression))
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
if (!_isSearchCondition && !(sqlUnaryExpression.Operand is ExistsExpression or InExpression or LikeExpression))
if (!_isSearchCondition && sqlUnaryExpression.Operand is not (ExistsExpression or InExpression or LikeExpression)))

@ranma42 ranma42 marked this pull request as ready for review June 24, 2024 21:25
Copy link

@gabolarraguivel gabolarraguivel left a comment

Choose a reason for hiding this comment

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

Si funciona adelanet

@maumar
Copy link
Contributor

maumar commented Jun 27, 2024

@ranma42 consider adding a case with nullable bool in projection, something like:

    [ConditionalTheory]
    [MemberData(nameof(IsAsyncData))]
    public virtual Task Select_inverted_nullable_boolean(bool async)
        => AssertQuery(
            async,
            ss => ss.Set<CogTag>().Select(w => new { w.Id, Manual = (bool?)!w.Gear.HasSoulPatch }),
            ss => ss.Set<CogTag>().Select(w => new { w.Id, Manual = (bool?)(w.Gear == null ? null : !w.Gear.HasSoulPatch) }),
            elementSorter: e => e.Id);

this actually returns different results with XOR translation when HasSoulPatch (when Gear is null which causes HasSoulPatch to be null, we used to return false and now we return null). I would argue the new translation is the correct one (and on top of consistent with other providers that don't use search condition transformation). Basically it's the case pointed out in #34001, so maybe link this issue as a fix for 34001?

@maumar
Copy link
Contributor

maumar commented Jun 27, 2024

Hmm now that I think of it, 34001 probably covers other cases not fixed by this,
e.g.

ss => ss.Set<CogTag>().Select(w => new { w.Id, Manual = (bool?)!EF.Functions.IsNumeric(w.Gear.FullName) })

still produces

SELECT [t].[Id], CASE
    WHEN ISNUMERIC([g].[FullName]) <> 1 THEN CAST(1 AS bit)
    ELSE CAST(0 AS bit)
END AS [Manual]
FROM [Tags] AS [t]
LEFT JOIN [Gears] AS [g] ON [t].[GearNickName] = [g].[Nickname] AND [t].[GearSquadId] = [g].[SquadId]

and returns true for the null value of FullName. So we should keep #34001 open, until we have a proper fix (needs nullability information flow into search condition?)

But I think we should track this change somehow, for the purpose of accurately cataloguing all the breaking changes we do in this space. Thoughts? @roji @ranma42

@ranma42
Copy link
Contributor Author

ranma42 commented Jun 27, 2024

@maumar You are right, I did not think about #34001 when developing this change, but indeed it fixes this for some simple cases (basically a NOT of a BIT expression, even if nullable).
I updated the first message to mention this; I will update the branch with the test ASAP.

As you noticed, it does not fix it for generic Boolean expressions. Currently I only have an heavyweight solution for that (duplicating the condition) and AFAICT it is even a case in which CTE would be of limited use.
Knowing the nullability would help in several cases (basically, whenever we know that the expression is not nullable we could avoid the duplication), but I am afraid that the correct translation for the generic case will indeed be quite ugly.
(I am evaluating several tricks, including taking advantage of NULL checks in the CASE, which is quite likely to simplify a lot the expression if nullability is taken into account)

@ranma42
Copy link
Contributor Author

ranma42 commented Jun 27, 2024

@maumar I added a test similar to the one yup proposed; the main difference is that it operates directly on a bool? column, as that removes the need to choose a (different) expected expression.

LocustHorde has IsEradicated which since 9a80b6b is populated with all possible values (true, false, null).

public virtual Task Select_inverted_nullable_boolean(bool async)
=> AssertQuery(
async,
ss => ss.Set<LocustHorde>().Select(w => new { w.Id, Manual = !w.Eradicated }),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Alive = !w.Eradicated

When neither the parent expression nor the inner one is a predicate, translate to:
```sql
x ^ CAST(1 AS bit)
```

instead of

```sql
CASE
    WHEN x = CAST(0 AS bit) THEN CAST(1 AS bit)
    ELSE CAST(0 AS bit)
END
```

Contributes to dotnet#34001 for simple cases (NOT of BIT expressions).
@maumar maumar merged commit 051c33a into dotnet:main Jun 28, 2024
7 checks passed
@maumar
Copy link
Contributor

maumar commented Jun 28, 2024

thanks a lot again @ranma42 ! Added a breaking-change label so that we don't forget to document this. (although it's more like a bug fix rather than a break, really :) )

@ranma42
Copy link
Contributor Author

ranma42 commented Jun 28, 2024

agreed; ideally this should only be a minor translation improvement, but since part of the translation is currently broken (NULL mapped to FALSE) and some users might be relying on that (maybe even unintentionally), I believe it is safer to warn about such a change (the same would hold for a fix to the general issue in #34001)

@ranma42 ranma42 deleted the not-xor-simplify branch June 28, 2024 20:34
maumar pushed a commit that referenced this pull request Jul 29, 2024
Since #34080 the SqlServer provider uses XOR to avoid converting to/from predicates when negating BIT expressions.
It is actually possible to simply use ~ on BIT values and this changeset implements this.

Fixes #34213

* Add minimal support for `ExpressionType.OnesComplement` It is currently only supported in the final part of SqlServer translation pipeline.
* Use `~` for negating boolean values Instead of `x ^ 1`, use the `~x` expression when translating boolean values in SqlServer.
@roji roji changed the title Use XOR to translate some NOT expressions SQL Server: Use XOR to translate some NOT expressions Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants