CSHARP-5931: Add builder support for $scoreFusion stage#1976
CSHARP-5931: Add builder support for $scoreFusion stage#1976adelinowona wants to merge 3 commits intomongodb:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds C# driver support for building and executing the $scoreFusion aggregation stage, including options/metadata types and test coverage.
Changes:
- Introduces
PipelineStageDefinitionBuilder.ScoreFusion(named, auto-named, and weighted tuple overloads) plus shared helpers. - Adds public API surface for fluent aggregation and pipeline builders (
IAggregateFluentExtensions,PipelineDefinitionBuilder) and new options/metadata types. - Adds unit and integration tests and gates integration coverage behind a new server
Feature.ScoreFusionStage.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/MongoDB.Driver.Tests/PipelineStageDefinitionBuilderTests.cs | Adds rendering/validation unit tests for $scoreFusion stage builder. |
| tests/MongoDB.Driver.Tests/PipelineDefinitionBuilderTests.cs | Adds null-guard tests for pipeline extension methods (pipeline.ScoreFusion(...)). |
| tests/MongoDB.Driver.Tests/AggregateScoreFusionTests.cs | Adds integration tests for $scoreFusion end-to-end behavior and metadata projection. |
| src/MongoDB.Driver/ScoreFusionScoreDetails.cs | Adds POCO for scoreDetails metadata returned by $scoreFusion. |
| src/MongoDB.Driver/ScoreFusionOptions.cs | Adds normalization/combination enums and ScoreFusionOptions<T> configuration object. |
| src/MongoDB.Driver/PipelineStageDefinitionBuilder.cs | Implements $scoreFusion stage builder and refactors shared named/auto-named pipeline helpers. |
| src/MongoDB.Driver/PipelineDefinitionBuilder.cs | Adds pipeline extension methods to append $scoreFusion. |
| src/MongoDB.Driver/IAggregateFluentExtensions.cs | Adds IAggregateFluent extensions for $scoreFusion. |
| src/MongoDB.Driver/Core/Misc/Feature.cs | Adds Feature.ScoreFusionStage to gate integration tests by server wire version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
adelinowona
changed the title
BorisDog
left a comment
BorisDog
left a comment
There was a problem hiding this comment.
Looks good overall.
Please change the Jira ticket title to match the PR title.
| throw new ArgumentException("Value cannot be an empty collection.", nameof(pipelines)); | ||
| } | ||
|
|
||
| if (pipelines.Any(pipeline => pipeline == null)) |
There was a problem hiding this comment.
Maybe validate once in BuildAutoNamedPipelineMap ?
There was a problem hiding this comment.
Done. Moved the null-pipeline check into BuildAutoNamedPipelineMap and BuildAutoNamedPipelineMapWithWeights as a pre-check before building the map. This also benefits the RankFusion overloads that share these helpers.
Note: it'll be behavioral breaking change for RankFusion as the error checking is now done client-side instead of being deferred to the server.
There was a problem hiding this comment.
I think it's fine to fail earlier in this case.
There was a problem hiding this comment.
Just found Ensure.IsNotNullAndDoesNotContainAnyNulls
| /// <returns>The fluent aggregate interface.</returns> | ||
| public static IAggregateFluent<TNewResult> ScoreFusion<TResult, TNewResult>( | ||
| this IAggregateFluent<TResult> aggregate, | ||
| Dictionary<string, PipelineDefinition<TResult, TNewResult>> pipelines, |
There was a problem hiding this comment.
Should we use IReadOnlyDictionary<> in public API instead of Dictionary<>?
There was a problem hiding this comment.
Probably but changing this would require updating RankFusion too for consistency, which would be a breaking change there. We could defer this to a major release where both can be updated together? WDYT @BorisDog @ajcvickers
There was a problem hiding this comment.
Good idea, and major release sgtm.
BorisDog
left a comment
BorisDog
left a comment
There was a problem hiding this comment.
LGTM + minor comment about Ensure.IsNotNullAndDoesNotContainAnyNulls
| /// <returns>The fluent aggregate interface.</returns> | ||
| public static IAggregateFluent<TNewResult> ScoreFusion<TResult, TNewResult>( | ||
| this IAggregateFluent<TResult> aggregate, | ||
| Dictionary<string, PipelineDefinition<TResult, TNewResult>> pipelines, |
There was a problem hiding this comment.
Good idea, and major release sgtm.
| throw new ArgumentException("Value cannot be an empty collection.", nameof(pipelines)); | ||
| } | ||
|
|
||
| if (pipelines.Any(pipeline => pipeline == null)) |
There was a problem hiding this comment.
I think it's fine to fail earlier in this case.
| throw new ArgumentException("Value cannot be an empty collection.", nameof(pipelines)); | ||
| } | ||
|
|
||
| if (pipelines.Any(pipeline => pipeline == null)) |
There was a problem hiding this comment.
Just found Ensure.IsNotNullAndDoesNotContainAnyNulls
No description provided.