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.
| 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?
src/MongoDB.Driver/RerankQuery.cs
Outdated
| /// <param name="text">The text to rerank against.</param> | ||
| /// <returns>A text rerank query.</returns> | ||
| public static RerankQuery Text(string text) => | ||
| new RerankQuery(new BsonDocument("text", Ensure.IsNotNull(text, nameof(text)))); |
There was a problem hiding this comment.
I was going to defer to the server validation but I suppose the early catch is worth it here.
src/MongoDB.Driver/RerankQuery.cs
Outdated
| public static RerankQuery Text(string text) => | ||
| new RerankQuery(new BsonDocument("text", Ensure.IsNotNull(text, nameof(text)))); | ||
|
|
||
| internal BsonDocument Render() => _rendered; |
There was a problem hiding this comment.
I'm not sure if it's safe to have the pre-rendered document here. Because we return it as a BsonDocument which is mutable, it means it might be populated/altered (I do not know, for example we could add _id field), reusing the same BsonDocument might lead to some weird behavior. I would suggest to render to a new document on each call.
There was a problem hiding this comment.
Nice catch, will do.
| } | ||
|
|
||
| [Fact] | ||
| public void Rerank_with_incorrect_params_should_throw_expected_exception() |
There was a problem hiding this comment.
We should split this to several Facts validating each parameter separately. Or a theory with test case for each parameter.
There was a problem hiding this comment.
will separate to different Facts
No description provided.