CSHARP-5473: Provide API to turn LINQ expression into MQL. #1601
Conversation
rstam
requested review from
JamesKovacs,
damieng,
BorisDog,
sanych-sun and
adelinowona
| public class CSharp5473Tests : Linq3IntegrationTest | ||
| { | ||
| [Fact] | ||
| public void Select_decimal_divide_should_work() |
There was a problem hiding this comment.
Minor : Test method name seems to be wrong.
| var stages = provider.Translate(queryable, out var outputSerializer); | ||
| AssertStages(stages, "{ $project : { _v : { $add : ['$X', 1] }, _id : 0 } }"); | ||
|
|
||
| var result = queryable.First(); |
There was a problem hiding this comment.
Should we try to execute translated MQL in some way instead to make sure the translation was done properly?
There was a problem hiding this comment.
Yes, but not necessarily to verify that the translation was done correctly.
I'm more interested in showing HOW you could execute the result of Translate.
| @@ -152,5 +153,14 @@ public override ExpressionTranslationOptions GetTranslationOptions() | |||
| var database = _database ?? _collection?.Database; | |||
| return translationOptions.AddMissingOptionsFrom(database?.Client.Settings.TranslationOptions); | |||
| } | |||
|
|
|||
| public override BsonDocument[] Translate<TResult>(IQueryable<TResult> queryable, out IBsonSerializer<TResult> outputSerializer) | |||
There was a problem hiding this comment.
Should we check if provided queryable is MongoDB queryable?
There was a problem hiding this comment.
Or maybe receiving an expression is sufficient?
There was a problem hiding this comment.
It would be sufficient.
I chose IQueryable to emphasize that it's the Expression associated with an IQueryable that is being translated, not any old expression.
But the .NET method takes an Expression, so we could also here.
@damieng what do you think?
There was a problem hiding this comment.
The translate method seems to only care about the expression of the queryable so I think we should just take Expression here as well. We can add a note in the comments of methods to emphasize the expression should be associated with an IQueryable and maybe throw an exception if we catch such as a case?
|
I think if we want to ship this for now we should use the Alternatively I could use reflection to do the same sort of thing on the already-released version and make sure it meets our use cases on the EF provider before shipping. |
You are the one that asked for this. If you are not sure it does what you need then we shouldn't ship it yet. |
…instead of an IQueryable.
| /// <param name="outputSerializer">The output serializer.</param> | ||
| /// <typeparam name="TResult">The type of the result.</typeparam> | ||
| /// <returns>An array of MQL pipeline stages represented as BsonDocuments.</returns> | ||
| BsonDocument[] Translate<TResult>(Expression expression, out IBsonSerializer<TResult> outputSerializer); |
There was a problem hiding this comment.
There was a problem hiding this comment.
I think receiving expression is more consistent with ExecuteAsync.
Analyzer will not use this functionality at all.
The BIG question for this PR is whether we want to do this at all, and if so what the public API should be.