CSHARP-5478: Support $rankFusion stage #1636
Conversation
adelinowona
requested review from
JamesKovacs,
rstam and
BorisDog
and removed request for
a team and
JamesKovacs
| /// <summary> | ||
| /// Gets the $rankFusion feature. | ||
| /// </summary> | ||
| public static Feature RankFusion => __rankFusion; |
There was a problem hiding this comment.
Feature names for stages end in Stage. Rename to RankFusionStage?
| /// <typeparam name="TNewResult">The type of the new result.</typeparam> | ||
| /// <param name="pipelines">The map of named pipelines whose results will be combined. The pipelines must operate on the same collection.</param> | ||
| /// <param name="weights">The map of pipeline names to numerical weights determining result importance during combination. Default weight is 1 when unspecified.</param> | ||
| /// <param name="scoreDetails">Flag that specifies whether to return a detailed breakdown of the score for each document in the result.</param> |
There was a problem hiding this comment.
I'm not sure this description is correct, but I'm also not confident how $rankFusion works exactly.
My question is that based on a Slack conversation I don't think setting this option to true changes the result at all. What I understood from Adelin is that setting it to true causes the $meta variable to be defined, and it's up to the user whether to project that into the result or not.
There was a problem hiding this comment.
I'll update the description to mention setting the options to true doesn't change the resulting POCO but makes the score available as a new metadata field that can be projected by the user using $meta.
| return new RenderedPipelineStageDefinition<TOutput>( | ||
| operatorName, | ||
| new BsonDocument(operatorName, rankFusionOptions), | ||
| args.SerializerRegistry.GetSerializer<TOutput>()); |
There was a problem hiding this comment.
Is the registry the right place to get this serializer from?
Sometimes we allow the user to tell us what serializer to use in an options parameter, and only fall back to the registry if none was provided.
There was a problem hiding this comment.
That's a good point. I just noticed that some of the methods in PipelineStageDefinitionBuilder relied upon registry and I did as well but it is probably the right call to allow users to provide a serializer if they want and fall back to the registry if none if provided.
| /// </summary> | ||
| /// <typeparam name="TNewResult">The type of the new result.</typeparam> | ||
| /// <param name="pipelines">The map of named pipelines whose results will be combined. The pipelines must operate on the same collection.</param> | ||
| /// <param name="weights">The map of pipeline names to numerical weights determining result importance during combination. Default weight is 1 when unspecified.</param> |
There was a problem hiding this comment.
"non-negative numerical weights"
| renderedPipelines.Add(pipeline.Key, new BsonArray(pipeline.Value.Render(args).Documents)); | ||
| } | ||
|
|
||
| var rankFusionOptions = new BsonDocument |
There was a problem hiding this comment.
minor: consider renaming rankFusionOptions to document for consistency with other operators.
| Dictionary<string, PipelineDefinition<TResult, TNewResult>> pipelines, | ||
| Dictionary<string, double> weights = null, | ||
| bool scoreDetails = false) | ||
| { |
There was a problem hiding this comment.
Consider the following convenience overloads in IAggregateFluentExtensions
RankFusion<TNewResult>(PipelineDefinition<TResult, TNewResult>[] pipelines, options)
RankFusion<TNewResult>((PipelineDefinition<TResult, TNewResult>, double)[] pipelinesWithWeights, options)
cc @rstam
| result[1].Y.Should().Be(7); | ||
| result[2].Y.Should().Be(5); | ||
| result[3].Y.Should().Be(8); | ||
| } |
There was a problem hiding this comment.
Let's add some basic validation for scoreDetails, it will also serve as example on how to consume it.
No description provided.