Optimize DISTINCT over collections of 0 or 1 element #34702
Conversation
| @@ -255,6 +255,11 @@ public void ApplyDistinct() | |||
| throw new InvalidOperationException(RelationalStrings.DistinctOnCollectionNotSupported); | |||
| } | |||
|
|
|||
| if (Limit is SqlConstantExpression { Value: 0 or 1 }) | |||
There was a problem hiding this comment.
does it make sense to handle the case of 0? AFAICT SELECT expressions with a LIMIT 0 could be optimized away heavily (basically only the projection/typing matters; offset, predicate, distinct, ... are all irrelevant)
There was a problem hiding this comment.
Yeah, I agree - if we want to optimize the LIMIT 0 case, we should probably do that separately and go much farther than just removing DISTINCT... Any thoughts on cases where LIMIT 0 could be an actually useful thing to do (and therefore worth working on)?
| @@ -1901,6 +1906,10 @@ public void ApplyLimit(SqlExpression sqlExpression) | |||
| } | |||
|
|
|||
| Limit = sqlExpression; | |||
| if (Offset is null && Limit is SqlConstantExpression { Value: 0 or 1 }) | |||
There was a problem hiding this comment.
nit:
| if (Offset is null && Limit is SqlConstantExpression { Value: 0 or 1 }) | |
| if (Offset is null && Limit is SqlConstantExpression { Value: 0 or 1 }) |
| @@ -255,6 +255,11 @@ public void ApplyDistinct() | |||
| throw new InvalidOperationException(RelationalStrings.DistinctOnCollectionNotSupported); | |||
| } | |||
|
|
|||
| if (Limit is SqlConstantExpression { Value: 0 or 1 }) | |||
There was a problem hiding this comment.
Yeah, I agree - if we want to optimize the LIMIT 0 case, we should probably do that separately and go much farther than just removing DISTINCT... Any thoughts on cases where LIMIT 0 could be an actually useful thing to do (and therefore worth working on)?
This does some progress towards fixing #34482, but in several cases the optimization does not trigger because of another intermediate operation.
I will investigate whether it makes sense to combine these operations in a separate PR or if they can be easily handled in this one.
The
Single/First[OrDefault]case seems to be reasonably tested; for theTake(1)case it might be better to add a new test (it is tested, but AFAICT only in combination withSingle/First[OrDefault]).