-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CSHARP-4627: Support Union via $unionWith in LINQ3 #1078
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
...mplementation/Translators/ExpressionToPipelineTranslators/UnionMethodToPipelineTranslator.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work. I have some non-trivial refactoring suggestions based on my experience writing LINQ3 translators. Let's meet on Zoom tomorrow to discuss.
throw new ExpressionNotSupportedException(innerExpression, containerExpression, because: message); | ||
} | ||
|
||
public static bool TryGetCollectionInfo(this Expression innerExpression, out string collectionName, out IBsonSerializer documentSerializer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead of this change we need a new extension method called Evaluate:
public static object Evaluate(this Expression expression)
{
if (expression is ConstantExpression constantExpression)
{
return constantExpression.Value;
}
else
{
LambdaExpression lambda = Expression.Lambda(expression);
Delegate fn = lambda.Compile();
return fn.DynamicInvoke(null);
}
}
See below for where it is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -67,6 +67,8 @@ public static AstPipeline Translate(TranslationContext context, Expression expre | |||
return TakeMethodToPipelineTranslator.Translate(context, methodCallExpression); | |||
case "Where": | |||
return WhereMethodToPipelineTranslator.Translate(context, methodCallExpression); | |||
case "Union": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: place in alphabetical order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
internal static class UnionMethodToPipelineTranslator | ||
{ | ||
// public static methods | ||
public static AstPipeline Translate(TranslationContext context, MethodCallExpression expression) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would propose the following refactoring:
public static AstPipeline Translate(TranslationContext context, MethodCallExpression expression)
{
var method = expression.Method;
var arguments = expression.Arguments;
if (method.Is(QueryableMethod.Union))
{
var firstExpression = arguments[0];
var pipeline = ExpressionToPipelineTranslator.Translate(context, firstExpression);
var secondExpression = arguments[1];
var secondValue = secondExpression.Evaluate();
if (secondValue is IMongoQueryable secondQueryable)
{
var secondProvider = secondQueryable.Provider;
var secondCollectionName = secondProvider.CollectionNamespace.CollectionName;
var secondPipelineInputSerializer = secondProvider.PipelineInputSerializer;
var secondContext = TranslationContext.Create(secondQueryable.Expression, secondPipelineInputSerializer);
var secondPipeline = ExpressionToPipelineTranslator.Translate(secondContext, secondQueryable.Expression);
if (secondPipeline.Stages.Count == 0)
{
secondPipeline = null;
}
pipeline = pipeline.AddStages(
pipeline.OutputSerializer,
AstStage.UnionWith(secondCollectionName, secondPipeline));
return pipeline;
}
else if (secondValue is IQueryable)
{
throw new ExpressionNotSupportedException(expression, because: "second argument must be IMongoQueryable, not just IQueryable");
}
else
{
// todo: handle enumerable constant using $documents?
}
}
throw new ExpressionNotSupportedException(expression);
}
Let's discuss over Zoom tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
...mplementation/Translators/ExpressionToPipelineTranslators/UnionMethodToPipelineTranslator.cs
Outdated
Show resolved
Hide resolved
.Union(_secondCollection.AsQueryable()); | ||
|
||
var stages = Translate(_firstCollection, queryable); | ||
AssertStages(stages, "{ $unionWith : { coll : 'partners', pipeline : [] } }"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AssertStages(stages, "{ $unionWith : 'partners' }");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
var stages = Translate(_firstCollection, queryable); | ||
AssertStages(stages, "{ $unionWith : { coll : 'partners', pipeline : [] } }"); | ||
|
||
var items = queryable.ToList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We normally name the results variable results
(same for other test methods).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
.Union(_firstCollection.AsQueryable()); | ||
|
||
var stages = Translate(_firstCollection, queryable); | ||
AssertStages(stages, "{ $unionWith : { coll : 'clients', pipeline : [] } }"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AssertStages(stages, "{ $unionWith : 'clients' }");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
99% there
@@ -44,5 +45,19 @@ public static TValue GetConstantValue<TValue>(this Expression expression, Expres | |||
var message = $"Expression must be a constant: {expression} in {containingExpression}."; | |||
throw new ExpressionNotSupportedException(message); | |||
} | |||
|
|||
public static object Evaluate(this Expression expression) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general rule we keep methods in alphabetical order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return pipeline; | ||
} | ||
|
||
throw new ExpressionNotSupportedException(expression, because: "second argument must be IMongoQueryable, not just IQueryable"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slight change to exception message:
throw new ExpressionNotSupportedException(expression, because: "second argument must be IMongoQueryable");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
new Company { Id = 5, Name = "another partner" }); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete extra blank line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
.AsQueryable() | ||
.Where(c => c.Name.StartsWith("second")) | ||
.Union(_secondCollection.AsQueryable() | ||
.Where(c => c.Name.StartsWith("another"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested formatting:
var queryable = _firstCollection
.AsQueryable()
.Where(c => c.Name.StartsWith("second"))
.Union(_secondCollection.AsQueryable().Where(p => p.Name.StartsWith("another")));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Looks great!
No description provided.