SqlServer: Generate False predicate when skip/take both are 0 #23192
Conversation
|
Hello @smitpatel! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
src/EFCore.SqlServer/Query/Internal/SkipTakeCollapsingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
| @@ -3152,12 +3154,12 @@ private bool Equals(SelectExpression selectExpression) | |||
| /// <returns> This expression if no children changed, or an expression with the updated children. </returns> | |||
| // This does not take internal states since when using this method SelectExpression should be finalized | |||
| public SelectExpression Update( | |||
| [NotNull] List<ProjectionExpression> projections, | |||
| [NotNull] List<TableExpressionBase> tables, | |||
| [NotNull] IReadOnlyList<ProjectionExpression> projections, | |||
There was a problem hiding this comment.
Breaking change, including in obsolete method above?
There was a problem hiding this comment.
It is not breaking change if you follow the bases.
There was a problem hiding this comment.
@AndriySvyryd - Is this breaking change? Not sure how this missed API review.
For the obsolete one, it added additional NotNulls but the overload is already obsolete, not sure what would be the way to unbreak it.
There was a problem hiding this comment.
The type change is indeed only a provider-facing binary breaking change only, which should be fine. But if this method used to work for null parameters and doesn't any more, that is more problematic.
There was a problem hiding this comment.
Not this method. This method was always marked with NotNull and has Check.NotNull for all the non-nullable arguments.
src/EFCore.SqlServer/Query/Internal/SkipTakeCollapsingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
| @@ -2326,6 +2327,35 @@ public virtual Task GroupBy_aggregate_join_another_GroupBy_aggregate(bool async) | |||
| elementSorter: o => o.Key); | |||
| } | |||
|
|
|||
| [ConditionalTheory] | |||
| [MemberData(nameof(IsAsyncData))] | |||
| public virtual Task GroupBy_aggregate_after_skip_0_take_0(bool async) | |||
There was a problem hiding this comment.
Not sure, but as this is a SQL Server problem, should these tests live exclusively in the SQL Server test suite?
There was a problem hiding this comment.
If it works in other providers then what is the issue verifying that?
There was a problem hiding this comment.
Just the fact of adding unnecessary tests to all providers for something that is only a problem for one. I have a lot of PG-only tests which aren't added to the base classes only because EFCore.PG isn't in the EF Core repo, and SQL Server is.
It's also a form of documentation for someone reviewing the tests, to note which provider is actually affected by it etc. Just putting it in the right place.
But no strong feelings here.
smitpatel
force-pushed
the
smit/Skip0Take0
branch
from
83b4a78 to
29b62ca
Compare
ghost
merged commit 
|
Oops, forgot to remove auto-merge :/ If needed we can discuss/amend post-merge. |
Resolves #22525