CSHARP-4443: Add comprehensive dictionary LINQ support for all 3 representations #1804

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.

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.

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

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.

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?

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?

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 requested 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 requested changes Dec 13, 2025

View reviewed changes

CSHARP-4443: Add comprehensive dictionary LINQ support for all 3 representations #1804

Are you sure you want to change the base?

CSHARP-4443: Add comprehensive dictionary LINQ support for all 3 representations #1804

Uh oh!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants