CSHARP-5828: Add Rerank stage builder#1936
Conversation
adelinowona
requested review from
Copilot,
kyra-rk and
sanych-sun
and removed request for
kyra-rk
There was a problem hiding this comment.
Pull request overview
Adds first-class support for building/appending a MongoDB aggregation $rerank stage across the stage builder, pipeline builder extensions, and fluent aggregate API.
Changes:
- Added
$rerankstage builders toPipelineStageDefinitionBuilder. - Added pipeline extension methods to append
$rerankstages viaPipelineDefinitionBuilder. - Added fluent aggregate API surface for
$rerankplus new unit tests covering rendering and parameter validation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/MongoDB.Driver.Tests/PipelineStageDefinitionBuilderTests.cs | Adds argument-validation tests for the new stage builder APIs. |
| tests/MongoDB.Driver.Tests/PipelineDefinitionBuilderTests.cs | Adds rendering tests for $rerank pipeline extension methods and updates test model. |
| src/MongoDB.Driver/PipelineStageDefinitionBuilder.cs | Introduces $rerank stage construction and rendering logic. |
| src/MongoDB.Driver/PipelineDefinitionBuilder.cs | Adds pipeline extension methods to append $rerank stages. |
| src/MongoDB.Driver/IAggregateFluentExtensions.cs | Adds expression-based fluent extension overload for $rerank. |
| src/MongoDB.Driver/IAggregateFluent.cs | Extends the fluent aggregate interface with a $rerank stage method. |
| src/MongoDB.Driver/AggregateFluentBase.cs | Adds base virtual method stub for $rerank. |
| src/MongoDB.Driver/AggregateFluent.cs | Implements $rerank by delegating to the underlying pipeline. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
adelinowona
force-pushed
the
csharp5828
branch
2 times, most recently
from
fc62d55 to
07167c7
Compare
| public static PipelineDefinition<TInput, TOutput> Rerank<TInput, TOutput>( | ||
| this PipelineDefinition<TInput, TOutput> pipeline, | ||
| RerankQuery query, | ||
| IEnumerable<FieldDefinition<TOutput>> paths, |
There was a problem hiding this comment.
Let's either use IEnumerable parameter or params in both cases: here and on the line 1126.
There was a problem hiding this comment.
The two multi-path overloads serve different use cases: the params Expression[] one is for inline type-safe expressions (x => x.Title, x => x.Plot), and the IEnumerable<FieldDefinition> one is for pre-built or dynamic collections of string-based paths. Using params for FieldDefinition wouldn't add much since there's no type inference benefit, and using IEnumerable<Expression> would lose the params convenience. I think the inconsistency is justified by the different use cases here.
Thoughts?
There was a problem hiding this comment.
I see this as a different way to provide the same parameters. Having one as an array and another as a params looks confusing a little. About different use cases: both expressions and FieldDefinitions could be used as fixed list of values or dynamic collections I believe. I think I would prefer consistency between overloads if it's possible. @BorisDog @papafe @ajcvickers WDYT?
There was a problem hiding this comment.
Changing my mind, as I already commented. I don't agree with, "Having one as an array and another as a params looks confusing a little. " but I do like keeping the parameter order the same between the overloads.
| string model) | ||
| => Rerank( | ||
| query, | ||
| new ExpressionFieldDefinition<TInput>(path), |
There was a problem hiding this comment.
minor: probably need a null check for path here, for consistent exception with other overload.
There was a problem hiding this comment.
nit: You could keep the expression body and have
return Rerank(
query,
new ExpressionFieldDefinition<TInput>( Ensure.IsNotNull(path, nameof(path)))
| RerankQuery query, | ||
| int numDocsToRerank, | ||
| string model, | ||
| params Expression<Func<TOutput, TField>>[] paths) |
There was a problem hiding this comment.
Yeah as you mentioned the inconsistency in path and paths arguments position is not ideal. But no strong opinion.
Let's see what @ajcvickers thinks.
There was a problem hiding this comment.
I didn't notice the change in argument ordering before. I prefer the ordering where the paths are immediately after the query--this feels like what we commonly do with paths, and feels natural here like Adelin said. So, given that the params doesn't add a lot in modern C#, I think I would drop it and keep the order the same for overloads. So, yeah, I'm changing my mind. :-) But I don't feel strongly.
There was a problem hiding this comment.
I agree with Arthur here (and also don't feel strongly).
ajcvickers
left a comment
ajcvickers
left a comment
There was a problem hiding this comment.
LGTM, except I marginally prefer to drop the params array and keep the overloads having the same parameter order.
BorisDog
left a comment
BorisDog
left a comment
There was a problem hiding this comment.
LGTM + 2 minor suggestions.
| string model) | ||
| => Rerank( | ||
| query, | ||
| new ExpressionFieldDefinition<TInput>(path), |
There was a problem hiding this comment.
nit: You could keep the expression body and have
return Rerank(
query,
new ExpressionFieldDefinition<TInput>( Ensure.IsNotNull(path, nameof(path)))
No description provided.