CSHARP-4882: Support Skip and Take in expressions. #1584
Conversation
rstam
requested review from
JamesKovacs,
sanych-sun,
adelinowona and
damieng
and removed request for
a team and
JamesKovacs
| return args[0]; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
@sanych-sun let's discuss the pros and cons of doing these simplifications here vs in the AstSimplifier.
There was a problem hiding this comment.
Discussing this with Alex we have chosen to leave as is for the time being.
| @@ -229,13 +230,14 @@ protected override Expression VisitMethodCall(MethodCallExpression node) | |||
| var result = base.VisitMethodCall(node); | |||
|
|
|||
| var method = node.Method; | |||
| if (IsCustomLinqExtensionMethod(method)) | |||
| if (IsCustomLinqExtensionMethod(method) || | |||
| method.Is(QueryableMethod.AsQueryable)) | |||
There was a problem hiding this comment.
We can't let the PartialEvaluator evaluate nested AsQueryable.
@damieng here's an example of the headaches that nested AsQueryable causes.
| { | ||
| var sourceExpression = arguments[0]; | ||
| var countExpression = arguments[1]; | ||
| Expression skipExpression = null; | ||
| if (sourceExpression is MethodCallExpression sourceSkipExpression && sourceSkipExpression.Method.IsOneOf(__skipMethods)) |
There was a problem hiding this comment.
All simplifications (this was just one) have been moved to AstSimplifier.
| [InlineData(3, "{ $project : { _v : [2, 3, 4], _id : 0 } }", false)] | ||
| [InlineData(3, "{ $project : { _v : [2, 3, 4], _id : 0 } }", true)] | ||
| [InlineData(4, "{ $project : { _v : { $slice : [[1, 2, 3, 4], { $max : ['$One', 0] }, 2147483647] }, _id : 0 } }", false)] | ||
| [InlineData(4, "{ $project : { _v : { $slice : [[1, 2, 3, 4], { $max : ['$One', 0] }, 2147483647] }, _id : 0 } }", true)] |
There was a problem hiding this comment.
Testing nested AsQueryable doubles the number of tests.
@damieng here's another example of the headaches that nested AsQueryable causes.
| collection.AsQueryable().Select(x => new[] { 1, 2, 3, 4 }.Skip(1).ToArray()), | ||
| 4 => withNestedAsQueryable ? | ||
| collection.AsQueryable().Select(x => new[] { 1, 2, 3, 4 }.AsQueryable().Skip(x.One).ToArray()) : | ||
| collection.AsQueryable().Select(x => new[] { 1, 2, 3, 4 }.Skip(x.One).ToArray()), |
There was a problem hiding this comment.
Testing nested AsQueryable doubles the number of scenarios.
@damieng here's another example of the headaches that nested AsQueryable causes.
| [InlineData(7, "{ $project : { _v : { $slice : [{ $slice : [[1, 2, 3, 4], { $max : ['$One', 0] }, 2147483647] }, 2, 2147483647] }, _id : 0 } }", false)] | ||
| [InlineData(7, "{ $project : { _v : { $slice : [{ $slice : [[1, 2, 3, 4], { $max : ['$One', 0] }, 2147483647] }, 2, 2147483647] }, _id : 0 } }", true)] | ||
| [InlineData(8, "{ $project : { _v : { $slice : [{ $slice : [[1, 2, 3, 4], { $max : ['$One', 0] }, 2147483647] }, { $max : ['$Two', 0] }, 2147483647] }, _id : 0 } }", false)] | ||
| [InlineData(8, "{ $project : { _v : { $slice : [{ $slice : [[1, 2, 3, 4], { $max : ['$One', 0] }, 2147483647] }, { $max : ['$Two', 0] }, 2147483647] }, _id : 0 } }", true)] |
There was a problem hiding this comment.
I stopped modifying test methods to test nested AsQueryable with this one.
I'm wondering whether there is a better way to handle testing nested AsQueryable?
There was a problem hiding this comment.
Discussing with Alex we don't have a better way to test nested AsQueryable yet so I will refactor the remaining test methods to test nested AsQueryable also.
| QueryableMethod.Take, | ||
| }; | ||
|
|
||
| private static MethodInfo[] __skipOrTakeMethodsWithCount = |
There was a problem hiding this comment.
Is this the same array as __skipOrTakeMethods? Why do we need both?
No description provided.