efcore icon indicating copy to clipboard operation
efcore copied to clipboard

Implement `IS [NOT] DISTINCT FROM` translation

Open ranma42 opened this issue 1 year ago • 8 comments

Most SQL engines support the IS [NOT] DISTINCT FROM predicate, which conveniently matches C# semantics for !=/==. Where available (and efficient aka supported by indexes), using it can lead to simpler queries.

Fixes #10514, fixes #29624.

ranma42 avatar Jun 21 '24 07:06 ranma42

As mentioned in https://github.com/dotnet/efcore/issues/29624#issuecomment-2182047136 this is still WIP (opened as PR for viewing convenience @roji ).

ranma42 avatar Jun 21 '24 07:06 ranma42

Will require Compat level 160

ErikEJ avatar Jun 21 '24 09:06 ErikEJ

I will work more on this in the weekend. ⚠️ This changeset might cause is a significant reduction in the test coverage for RewriteNullSemantics. Will think about this in the weekend (maybe we can explicitly test for the interesting cases with a group of tests that intentionally "downgrade" the max sqlserver version).

ranma42 avatar Jun 21 '24 10:06 ranma42

I added the checks for SQLite version >= 3.8.11 and SqlServer CompatibilityVersion >= 160

Several tests are failing on SqlServer:

  • Compare_negated_bool_with_negated_bool_equal
  • Compare_bool_with_negated_bool_not_equal
  • Compare_negated_bool_with_bool_not_equal
  • Compare_negated_bool_with_bool_equal_negated
  • Compare_negated_bool_with_negated_bool_not_equal
  • Compare_negated_bool_with_bool_equal
  • Compare_bool_with_negated_bool_equal
  • Compare_bool_with_negated_bool_equal_negated
  • Compare_bool_with_negated_bool_not_equal_negated
  • Compare_negated_bool_with_negated_bool_equal_negated
  • Compare_negated_bool_with_bool_not_equal_negated
  • Compare_negated_bool_with_negated_bool_not_equal_negated

IIUC, this is because of https://github.com/dotnet/efcore/issues/34001

Note that currently the test have not yet been extended to check both the IS and the relational-null-replacement code paths.

ranma42 avatar Jun 22 '24 11:06 ranma42

I pushed a commit that avoids using IS [NOT] DISTINCT FROM when that would trigger #34001. Obviously it would still be desirable to (also) fix #34001

ranma42 avatar Jun 22 '24 13:06 ranma42

As an intermediate step, I opened #34072 which adds exhaustive testing of the null rewriting (plus some cleanup/minor improvements ;) ).

My current plan would be to run this test for SQL Server both normally and forcing a lower compatibility version for these tests (following the model of PrimitiveCollectionsQueryOldSqlServerTest).

ranma42 avatar Jun 23 '24 20:06 ranma42

I rebased this PR to resolve conflicts and to start getting to the issues that have been reported:

  • [x] rename IsExpression to IsNotDistinctFromExpression or IsDistinctFromExpression; I would like to keep the equality case as base, but I am afraid that IsNotDistinctFromExpression is complex (IsDistinctFromExpression has the disadvantage that it is the not-equal)
  • [x] add tests to ensure that there are no regressions in the null rewrite added in #34072
  • [x] check precedence of IS DISTINCT FROM in SqlServer
  • [x] remove bad translations of nullable bool? comparisons in SqlServer https://github.com/dotnet/efcore/pull/34048#discussion_r1650812620
  • [x] improve comparisons of nullable bool? by taking advantage of https://github.com/dotnet/efcore/pull/34080 (optional, desirable)
  • [ ] get the PR to pass the tests in CI
  • [x] introduce a reusable abstraction for providers (ApplyEqualityNullSemantics; might need some more polishing)
  • [ ] add an AppContext switch to opt out of the translation

ranma42 avatar Jun 29 '24 16:06 ranma42

I would like to keep the equality case as base, but I am afraid that IsNotDistinctFromExpression is complex (IsDistinctFromExpression has the disadvantage that it is the not-equal)

Yeah, the negated logic here is unfortunate. There's also the option of having two distinct expression types, though that would make it more complicated to handled in visitors needing to identify them.

roji avatar Jun 30 '24 13:06 roji

Rebased to solve conflicts and keep an eye on SQL Server updates in CI 😉

This is currently on top of https://github.com/dotnet/efcore/pull/34166 which affects the translation of boolean comparisons

ranma42 avatar Dec 23 '24 17:12 ranma42