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?
| 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.
| using MongoDB.Bson; | ||
| using MongoDB.Driver.Core.Misc; | ||
|
|
||
| namespace MongoDB.Driver |
There was a problem hiding this comment.
minor: filescoped namespaces.
| { | ||
| /// <summary> | ||
| /// Represents a query for the $rerank aggregation stage. | ||
| /// </summary> |
There was a problem hiding this comment.
minor: Could also add a reference to relevant builder in the doc.
| RerankQuery query, | ||
| Expression<Func<TInput, TField>> path, | ||
| int numDocsToRerank, | ||
| string model) |
There was a problem hiding this comment.
Double checking as docs are a bit confusing, model is not optional?
| /// <param name="text">The text to rerank against.</param> | ||
| /// <returns>A text rerank query.</returns> | ||
| public static RerankQuery Text(string text) => | ||
| new RerankQuery(Ensure.IsNotNullOrEmpty(text, nameof(text))); |
| { | ||
| PipelineStageDefinitionBuilder.Rerank<BsonDocument, string>(null, 10, "rerank-2.5"); | ||
| }).ParamName.Should().Be("query"); | ||
| } |
There was a problem hiding this comment.
Do we have a ticket for Atlas tests?
| 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.
No description provided.