CSHARP-4443: Add comprehensive dictionary LINQ support for all 3 representations by adelinowona · Pull Request #1804 · mongodb/mongo-csharp-driver

adelinowona · 2025-10-31T00:48:51Z

No description provided.

rstam

Note: I've only reviewed the tests, where I have a lot of suggestions about possible MQL for things you didn't implement or "better" MQL in some cases for things you did implement.

Please take all suggestions with a grain of salt. You may have reasons for thinking that what I think is "better" MQL might not actually be better.

I am postponing reviewing the implementation because if you accept many of the MQL suggestions you will need to make many changes to the implementation.

I will review the implementation after you have accepted/rejected the MQL suggestions.

rstam · 2025-10-31T19:36:14Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp2509Tests.cs

-
-namespace MongoDB.Driver.Tests.Linq.Linq3Implementation.Jira
-{
-    public class CSharp2509Tests : LinqIntegrationTest<CSharp2509Tests.ClassFixture>


I was surprised that these tests were deleted, but Adelin says they duplicate new tests he added in CSharp4443Tests.cs.

We don't usually remove tests... why bother?

Note to other reviewers: we have to manually verify that we haven't lost any test coverage as a result of deleting these tests.

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp4443Tests.cs

…esentations

Copilot

Pull request overview

This PR extends LINQ support for dictionary operations across all three BSON representations (Document, ArrayOfDocuments, and ArrayOfArrays). Previously, some operations like ContainsKey, ContainsValue, indexer access, and property access (Count, Keys, Values) only worked with the Document representation.

Key Changes:

Extended ContainsKey and ContainsValue to support ArrayOfDocuments and ArrayOfArrays representations
Added support for dictionary indexer access across all representations
Implemented dictionary property access (Count, Keys, Values) for all representations
Added support for KeyValuePair property access in array representation
Removed obsolete test files that tested the previous limited functionality

Reviewed changes

Copilot reviewed 24 out of 25 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`CSharp4813Tests.cs`	Removed dictionary-specific test methods that tested unsupported scenarios
`CSharp4557Tests.cs`	Deleted entire test file for ContainsKey with Document representation only
`CSharp2509Tests.cs`	Deleted entire test file for ContainsValue with limited representation support
`MemberExpressionToFilterFieldTranslator.cs`	Added KeyValuePair member translation and validation for Dictionary collections
`ContainsValueMethodToFilterTranslator.cs`	Extended to support ArrayOfArrays representation
`ContainsMethodToFilterTranslator.cs`	Added dictionary Keys/Values Contains support
`ContainsKeyMethodToFilterTranslator.cs`	Extended to support ArrayOfDocuments and ArrayOfArrays representations
`AllOrAnyMethodToFilterTranslator.cs`	Added validation to reject All/Any on Document representation dictionaries
`GetItemMethodToAggregationExpressionTranslator.cs`	Implemented indexer access for all dictionary representations
`ContainsValueMethodToAggregationExpressionTranslator.cs`	Refactored to extract reusable translation method
`ContainsMethodToAggregationExpressionTranslator.cs`	Added dictionary Keys/Values Contains support in aggregation expressions
`ContainsKeyMethodToAggregationExpressionTranslator.cs`	Extended to support all dictionary representations
`MemberExpressionToAggregationExpressionTranslator.cs`	Added comprehensive dictionary and KeyValuePair property translation
`AstExpression.cs`	Added `ArrayToObject` helper method
`KeyValuePairSerializer.cs`	Added `IKeyValuePairSerializerV2` interface with key/value serializer accessors

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

...ors/ExpressionToFilterTranslators/MethodTranslators/ContainsValueMethodToFilterTranslator.cs

adelinowona · 2025-11-26T19:55:25Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp5779Tests.cs

Moved the tests on Keys and Values properties of Dictionaries into this file since I think the ticket csharp5779 should have tested them.

Just a comment that resorting the entire file made a diff difficult.

rstam

Overall looking great. I would characterize most of my comments or requested changes as minor.

I haven't had time to review the tests yet, but wanted to give you the feedback I have so far.

Good work!

rstam · 2025-12-13T22:49:35Z

...ionExpressionTranslators/MethodTranslators/AverageMethodToAggregationExpressionTranslator.cs

                else
                {
-                    ast = AstExpression.Avg(sourceTranslation.Ast);
+                    var sourceItemSerializer = ArraySerializerHelper.GetItemSerializer(sourceTranslation.Serializer);


There is similar code in the translators for Max, Min and Sum.

Is there any way to share the code?

No problem if not.

Good point, added some code for sharing.

rstam · 2025-12-13T22:50:37Z

...onExpressionTranslators/MethodTranslators/ContainsMethodToAggregationExpressionTranslator.cs

+            Expression valueExpression,
+            out TranslatedExpression translation)
+        {
+            translation = null;


Same comment as above about assigning null to out parameter at top of method.

rstam · 2025-12-13T22:51:18Z

...onExpressionTranslators/MethodTranslators/ContainsMethodToAggregationExpressionTranslator.cs

+
+            if (!declaringType.IsGenericType ||
+                (declaringType.GetGenericTypeDefinition() != typeof(Dictionary<,>) &&
+                declaringType.GetGenericTypeDefinition() != typeof(IDictionary<,>)))


110-112 might be simpler like this:

if (!declaringType.ImplementsDictionaryInterface(out var keyType, out var valueType))

also: test with IReadOnlyDictionary

Good suggestion! After investigating, I think we can actually remove the filter entirely (lines 110-115).

The filter was originally meant to match what MemberExpressionTranslator supports for .Keys and .Values properties. However, in ContainsMethodToAggregationExpressionTranslator, we don't actually translate
those members - we just use them as a pattern match to identify that we should use ContainsKey/ContainsValue translation. That is, dictionary.Keys.Contains(Key) should have the same translation as dictionary.ContainsKey(key).

So we can remove the declaring type check entirely. This automatically enables support for IReadOnlyDictionary<,>, SortedDictionary<,>, and any other dictionary type with a proper serializer.

Does that sound reasonable?

rstam · 2025-12-13T22:53:04Z

...ssionToAggregationExpressionTranslators/MemberExpressionToAggregationExpressionTranslator.cs

+
+        private static bool TryTranslateKeyValuePairProperty(MemberExpression expression, TranslatedExpression container, MemberInfo memberInfo, out TranslatedExpression result)
+        {
+            result = null;


I don't like assigning null to out variables at the beginning of the method.

I realize it makes it more convenient to return false, but it also disables the compiler warning you get if you try to return true without setting an out parameter.

Your call.

That's fair, I updated it.

rstam · 2025-12-13T22:53:35Z

...ssionToAggregationExpressionTranslators/MemberExpressionToAggregationExpressionTranslator.cs

+
+            if (container.Expression.Type.IsGenericType &&
+                container.Expression.Type.GetGenericTypeDefinition() == typeof(KeyValuePair<,>) &&
+                container.Serializer is IKeyValuePairSerializerV2 { Representation: BsonType.Array } kvpSerializer)


Why are we ignoring Document representation here?

We're only handling Array representation in TryTranslateKeyValuePairProperty because Document representation is already correctly handled by the general member translation logic that follows (lines
94-108).

That code uses GetMemberSerializationInfo which returns the correct field names ("k" and "v") and serializers for Document-represented KeyValuePairs.

The method is only called when !AreMembersRepresentedAsFields (line 71), which is true for Array representation but false for Document representation.

rstam · 2025-12-13T22:58:08Z

...nslators/ExpressionToFilterTranslators/MethodTranslators/AllOrAnyMethodToFilterTranslator.cs


+                if (fieldTranslation.Serializer is IBsonDictionarySerializer { DictionaryRepresentation: DictionaryRepresentation.Document })
+                {
+                    throw new ExpressionNotSupportedException(expression);


throw new ExpressionNotSupportedException(expression, because: "dictionary is represented as a document");

Add because?

rstam · 2025-12-13T22:58:44Z

...ators/ExpressionToFilterTranslators/MethodTranslators/ContainsKeyMethodToFilterTranslator.cs

+                case DictionaryRepresentation.ArrayOfDocuments:
+                case DictionaryRepresentation.ArrayOfArrays:
+                    {
+                        var key = GetKeyStringConstant(expression, keyExpression, dictionarySerializer.KeySerializer);


If you move setting of key variable before the switch the braces would not be needed.

rstam · 2025-12-13T22:59:22Z

...nslators/ExpressionToFilterTranslators/MethodTranslators/ContainsMethodToFilterTranslator.cs

+            {
+                case "Keys":
+                {
+                    var dictionaryExpression = memberExpression.Expression;


If you move setting of dictionaryExpression variable before the switch the braces would not be needed.

rstam · 2025-12-13T22:59:52Z

...nslators/ExpressionToFilterTranslators/MethodTranslators/ContainsMethodToFilterTranslator.cs

+
+            if (!declaringType.IsGenericType ||
+                (declaringType.GetGenericTypeDefinition() != typeof(Dictionary<,>) &&
+                declaringType.GetGenericTypeDefinition() != typeof(IDictionary<,>)))


This would probably be simpler:

if (!declaringType.ImplementsDictionaryInterface(out _, out _))

and test with IReadOnlyDictionary also?

rstam · 2025-12-13T23:00:16Z

...ssionToFilterTranslators/ToFilterFieldTranslators/MemberExpressionToFilterFieldTranslator.cs

+                (memberExpression.Type.GetGenericTypeDefinition() == typeof(Dictionary<,>.KeyCollection) ||
+                 memberExpression.Type.GetGenericTypeDefinition() == typeof(Dictionary<,>.ValueCollection)))
+            {
+                throw new ExpressionNotSupportedException(memberExpression);


Add because?

rstam · 2025-12-15T16:18:56Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp5779Tests.cs

    public CSharp5779Tests(ClassFixture fixture)
        : base(fixture)
    {
    }

    [Fact]
-    public void DictionaryAsArrayOfArrays_Keys_should_work()


Shouldn't you have kept this test along with the two new ones with Contains and Count?

Oh wait.... did you just move the old test in a way that makes the differ harder to do?

Yes, the test is there, I just moved it around when I sorted the file.

rstam · 2025-12-15T17:40:37Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp5779Tests.cs

Just a comment that resorting the entire file made a diff difficult.

rstam · 2025-12-16T15:51:11Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp5779Tests.cs

+            .Select(x => x.DictionaryAsDocumentOfNestedDictionaryAsArrayOfArrays.Values.SelectMany(n => n.Keys));
+
+        var stages = Translate(collection, queryable);
+        AssertStages(stages, "{ $project : { _v : { $reduce : { input : { $map : { input : { $objectToArray : '$DictionaryAsDocumentOfNestedDictionaryAsArrayOfArrays' }, as : 'n', in : { $map : { input : '$$n.v', as : 'kvp', in : { $arrayElemAt : ['$$kvp', 0] } } }  } }, initialValue : [], in : { $concatArrays : ['$$value', '$$this'] } }  }, _id : 0 } }");


Replace double spaces with single space.

I don't see any double spaces in this area of the code?

Look for them in the string constant.

- Extract common IWrappedValueSerializer unwrapping logic into helper - Add ExpressionToAggregationExpressionTranslatorHelper.CreateAggregationAstWithUnwrapping - Refactor Average, Sum, Max/Min translators to use shared helper - Improve error messages with 'because' parameter - CountComparisonExpressionToFilterTranslator: clarify size must be integer constant - MemberExpressionToFilterFieldTranslator: explain KeyCollection/ValueCollection limitation - Fix code style issues - Move out parameter initialization after if blocks for compiler safety (TryTranslateKeyValuePairProperty) - Fix indentation in DictionaryIndexerComparisonExpressionToFilterTranslator - Hoist common variables before switch statements (ContainsKey, Contains translators) - Remove Count <= 0 pattern support (Count is never negative)

rstam

I've reviewed just the changes from the last commit I looked at to the most recent commit.

Can't remember if I need to do a quick full review of the whole PR.

In the meantime, I have one small change to suggest.

rstam · 2026-01-09T20:26:29Z

src/MongoDB.Driver/Linq/Linq3Implementation/Ast/Optimizers/AstSimplifier.cs

+            {
+                return AstExpression.Size(mapExpression.Input);
+            }
+


I think this simplification could be applied more generally:

// { $size : { map : { input : <array>, as : <var> in : <expr> } } } => { $size : <array> } if (node.Operator == AstUnaryOperator.Size && arg is AstMapExpression mapExpression) { return AstExpression.Size(mapExpression.Input); }

rstam · 2026-01-12T20:07:08Z

I did a quick full review of all the changes in all the comments and I think the only comment pending on my part is the recent one about generalizing the $size simplification.

rstam

LGTM

adelinowona requested review from rstam and sanych-sun October 31, 2025 00:48

adelinowona requested a review from a team as a code owner October 31, 2025 00:48

adelinowona added the feature Adds new user-facing functionality. label Oct 31, 2025

rstam suggested changes Nov 4, 2025

View reviewed changes

adelinowona added 8 commits November 26, 2025 13:19

CSHARP-4443: Add comprehensive dictionary LINQ support for all 3 repr…

bc008db

…esentations

add tests and update/remove duplicated tests

462dc1c

rebase on main for keys and values work from robert

205eb56

update some mql based on robert's suggestions

4c2d0bb

add a new translator for dictionary indexer comparison cases

f1d98e4

split the tests by representation

94484c9

move keys and values tests to CSharp5779Tests.cs

a416316

rebase on main to pull in the filter limit optimization work

fdb6fa7

Copilot AI review requested due to automatic review settings November 26, 2025 19:46

adelinowona force-pushed the csharp4443 branch from 34f74ce to fdb6fa7 Compare November 26, 2025 19:46

Copilot AI reviewed Nov 26, 2025

View reviewed changes

...ors/ExpressionToFilterTranslators/MethodTranslators/ContainsValueMethodToFilterTranslator.cs Show resolved Hide resolved

adelinowona commented Nov 26, 2025

View reviewed changes

check filter limit support for indexer access

0cb78dd

adelinowona requested a review from rstam November 26, 2025 20:32

rstam suggested changes Dec 13, 2025

View reviewed changes

rstam reviewed Dec 16, 2025

View reviewed changes

adelinowona requested a review from rstam December 19, 2025 23:36

rstam and others added 4 commits January 5, 2026 03:33

CSHARP-4443: POC of unwrapping wrapped values in TranslateEnumerable.

cb800c4

update Values unwrapping tests

5530f89

add AstSimplifier optimization for $size with maps

1d1ee9b

allow non-string keys to work with array representations

2ae6720

rstam suggested changes Jan 9, 2026

View reviewed changes

generalize the $size with maps optimization

ac6d457

adelinowona requested a review from rstam January 12, 2026 22:45

rstam approved these changes Jan 13, 2026

View reviewed changes

Conversation

adelinowona commented Oct 31, 2025

Uh oh!

rstam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rstam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adelinowona Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rstam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

adelinowona Dec 18, 2025 •

edited

Loading