CSHARP-5495: Support $concatArrays and $setUnion aggregation accumulators#2014
CSHARP-5495: Support $concatArrays and $setUnion aggregation accumulators#2014papafe wants to merge 12 commits into
Conversation
…tors Map g.SelectMany(x => x.Arr) and g.SelectMany(x => x.Arr).Distinct() to the new $concatArrays and $setUnion accumulators inside $group, $bucket and $bucketAuto via two new pattern matches in AstGroupingPipelineOptimizer. Add ConcatArrays and SetUnion extension methods on ISetWindowFieldsPartitionExtensions for use inside $setWindowFields.
…etunion-accumulators # Conflicts: # src/MongoDB.Driver/Core/Misc/Feature.cs
There was a problem hiding this comment.
Pull request overview
Adds LINQ3 translation support for MongoDB’s $concatArrays and $setUnion accumulators/window operators, including new window-partition extension methods, AST operator wiring, serializer inference, and integration tests for both $setWindowFields and grouping/bucket scenarios.
Changes:
- Added
ConcatArrays/SetUnionwindow-partition extension methods and reflection/translation plumbing to render$concatArraysand$setUnion. - Updated grouping pipeline optimizer to recognize
SelectMany-shaped ASTs and rewrite them into$concatArrays/$setUnionaccumulators. - Expanded test coverage for new operators in window and grouping/bucket contexts, plus serializer-finder expectations.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Translators/ExpressionToAggregationExpressionTranslators/MethodTranslators/WindowMethodToAggregationExpressionTranslatorTests.cs | Adds window translation tests for $concatArrays and $setUnion. |
| tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Translators/ExpressionToAggregationExpressionTranslators/MethodTranslators/SelectManyMethodToAggregationExpressionTranslatorTests.cs | Adds grouping/bucket tests expecting $concatArrays/$setUnion accumulators; adjusts fixture data and existing tests. |
| tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/SerializerFinders/WindowTests.cs | Adds serializer resolution coverage for new window methods. |
| src/MongoDB.Driver/Linq/Linq3Implementation/Translators/ExpressionToAggregationExpressionTranslators/MethodTranslators/WindowMethodToAggregationExpressionTranslator.cs | Registers ConcatArrays/SetUnion as unary window methods and maps them to AST operators. |
| src/MongoDB.Driver/Linq/Linq3Implementation/Translators/ExpressionToAggregationExpressionTranslators/MethodTranslators/ConcatArraysMethodToAggregationExpressionTranslator.cs | New method translator entry point delegating to window translation when applicable. |
| src/MongoDB.Driver/Linq/Linq3Implementation/Translators/ExpressionToAggregationExpressionTranslators/MethodTranslators/SetUnionMethodToAggregationExpressionTranslator.cs | New method translator entry point delegating to window translation when applicable. |
| src/MongoDB.Driver/Linq/Linq3Implementation/Translators/ExpressionToAggregationExpressionTranslators/MethodCallExpressionToAggregationExpressionTranslator.cs | Routes ConcatArrays/SetUnion method calls to the new translators. |
| src/MongoDB.Driver/Linq/Linq3Implementation/SerializerFinders/SerializerFinderVisitMethodCall.cs | Adds serializer deduction logic for ConcatArrays/SetUnion window methods. |
| src/MongoDB.Driver/Linq/Linq3Implementation/Reflection/WindowMethod.cs | Adds WindowMethod.ConcatArrays and WindowMethod.SetUnion reflection bindings. |
| src/MongoDB.Driver/Linq/Linq3Implementation/Ast/Optimizers/AstGroupingPipelineOptimizer.cs | Adds optimizer rewrites to turn SelectMany-shaped $reduce patterns into $concatArrays/$setUnion accumulators. |
| src/MongoDB.Driver/Linq/Linq3Implementation/Ast/Expressions/AstUnaryWindowOperator.cs | Adds renderable window operator enum values for $concatArrays/$setUnion. |
| src/MongoDB.Driver/Linq/Linq3Implementation/Ast/Expressions/AstUnaryAccumulatorOperator.cs | Adds renderable accumulator operator enum values for $concatArrays/$setUnion. |
| src/MongoDB.Driver/Linq/ISetWindowFieldsPartitionExtensions.cs | Adds public LINQ window APIs: ConcatArrays and SetUnion. |
| src/MongoDB.Driver/Core/Misc/Feature.cs | Introduces feature gate ConcatArraysAndSetUnionAccumulators (WireVersion.Server81). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The grouping/bucket SelectMany rewrite into $concatArrays/$setUnion accumulators requires server 8.1. Since that query shape was already translatable (as $push + $reduce, which runs on any server), emit the accumulator form only when the compatibility level supports it; otherwise keep the $reduce fallback. The $setWindowFields window methods are new API and stay unconditional.
- Add missing closing brace in SelectMany_Distinct_in_BucketAuto else-branch expected stage - Assert exact UniqueTags contents instead of only HaveCount - Reorder ConcatArrays/SetUnion alphabetically in WindowMethod and unary overload set
| } | ||
|
|
||
| private bool ArrayAccumulatorsSupported => | ||
| Feature.ConcatArraysAndSetUnionAccumulators.IsSupported((_translationOptions?.CompatibilityLevel).ToWireVersion()); |
There was a problem hiding this comment.
I know it is probably outside of this PR scope, but should we consider to change the ToWireVersion, to return min supported wire version for the null parameter? Because otherwise it's hardcoded to 4.0, which is 2 versions under our lowest supported version.
| _translationOptions = translationOptions; | ||
| } | ||
|
|
||
| private bool ArrayAccumulatorsSupported => |
There was a problem hiding this comment.
Property name seems to be misleading a little. Especially when we are using it to decide if we can use setUnion.
There was a problem hiding this comment.
I agree, changed it.
|
|
||
| [Theory] | ||
| [InlineData(ServerVersion.Server80)] | ||
| [InlineData(ServerVersion.Server81)] |
There was a problem hiding this comment.
And here I'm confused again: If we run the test on MongoDB 5.0 and set compatibility level to 8.0 - should this test fail with some server side error? Should we skip the test on older versions?
There was a problem hiding this comment.
This test is more of a regression check. In this case we're testing that if we have a SelectMany with index, we do not convert it to an accumulator, otherwise the index information will be unbound and rejected by the server.
In this case we want to be sure that independently from the compatibility level, we do not use the new accumulators (that were added in version 8.1). Since we use the "old" MQL, this is compatible on all versions.
There was a problem hiding this comment.
If we set compatibility level to ServerVersion.Server81 shouldn't we use the new functionality?
There was a problem hiding this comment.
No, if we have SelectMany with index (g.SelectMany((x, i) => ...) we can't use the new accumulator, this is what the test is verifying. If we have a SelectMany without index (g.SelectMany( x => ...), then we can use the new accumulator.
| [Theory] | ||
| [InlineData(ServerVersion.Server81, true)] | ||
| [InlineData(ServerVersion.Server80, false)] | ||
| public void SelectMany_in_GroupBy_should_emit_accumulator_only_when_CompatibilityLevel_supports_it(ServerVersion compatibilityLevel, bool expectAccumulator) |
There was a problem hiding this comment.
Regarding this test and the following... These could be possibly considered superfluous, but they test that the MQL is actually independent from the server version but only depends on the compatibility leve.
There was a problem hiding this comment.
So this test actually does not make all the way to the database? I would say it's unusual... I would prefer to actually execute the request and assert MQL based on the expected version, and skip test if we are trying to set the compatibility level to the version which is above the actual server version.
There was a problem hiding this comment.
Yes, they do not go all the way to the DB, because setting up the compatibility level to an arbitrary value could generate an MQL that is not valid (for example compatibility level 8.1 on a 5.0 server).
If you think about a matrix of server version (less than 8.1, from 8.1) and compatibility level (server80, server81), all the other tests here are checking only the "diagonal", so when the compatibility level is the same as the server version. This test here instead is checking all the possible combinations, as we can also have compatibility level equal to server81 and the actual server version is 5.0.
I would say this test is more for completeness, but I can understand if we want to remove it and keep the more direct behaviour. What do you think?
No description provided.