Skip to content

CSHARP-4779: Support Dictionary(IEnumerable<KeyValuePair<TKey, TValue>> collection) constructor in LINQ… #1657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 30, 2025

Conversation

sanych-sun
Copy link
Member

No description provided.

@sanych-sun sanych-sun requested a review from a team as a code owner April 1, 2025 00:27
@sanych-sun sanych-sun requested review from papafe, rstam and adelinowona and removed request for a team and papafe April 1, 2025 00:27

var stages = Translate(collection, queryable);

AssertStages(stages, "{ $project : { _v : { $arrayToObject : [[{ k : 'A', v : '$A' }, { k : 'B', v : '$B' }]] }, _id : 0 } }");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A further simplifcation could turn this into:

{ $project : { _v : { A : "$A", B : "$B" }, _id : 0 } }

When all the k values passed to $arrayToObject are string constants you can do this simplification.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@adelinowona adelinowona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me, just left some minor comments.

Comment on lines 54 to 69
// [Fact]
// public void NewDictionary_with_KeyValuePairs_Create_should_translate()
// {
// var collection = Fixture.Collection;
//
// var queryable = collection.AsQueryable()
// .Select(d => new Dictionary<string, string>(
// new[] { KeyValuePair.Create("A", d.A), KeyValuePair.Create("B", d.B) }));
//
// var stages = Translate(collection, queryable);
//
// AssertStages(stages, "{ $project : { _v : { $arrayToObject : [[{ k : 'A', v : '$A' }, { k : 'B', v : '$B' }]] }, _id : 0 } }");
//
// var result = queryable.Single();
// result.Should().Equal(new Dictionary<string, string>{ ["A"] = "a", ["B"] = "b" });
// }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot to remove this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created another ticket to add support of KeyValuePair.Create. Then will uncomment this test. But thank you for reminding me not to merge like this =)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is uncommented now.

Comment on lines 79 to 80
if (keySerializer is not IRepresentationConfigurable representationConfigurableSerializer
|| representationConfigurableSerializer.Representation != BsonType.String)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be simplified to if (keySerializer is not IRepresentationConfigurable { Representation: BsonType.String })

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 44 to 45
if (itemSerializationInfo.Serializer is IRepresentationConfigurable representationConfigurable &&
representationConfigurable.Representation == BsonType.Array)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could also be simplified to if (itemSerializationInfo.Serializer is IRepresentationConfigurable { Representation: BsonType.Array })

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


var collectionExpression = arguments.Single();
var collectionTranslation = ExpressionToAggregationExpressionTranslator.TranslateEnumerable(context, collectionExpression);
AstExpression collectionTranslationAst;
Copy link
Contributor

@adelinowona adelinowona Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move this declaration inside the first if statement below

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@sanych-sun sanych-sun requested review from adelinowona and rstam April 1, 2025 23:14
Copy link
Contributor

@adelinowona adelinowona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@rstam rstam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed suggested changes offline.

@sanych-sun sanych-sun requested a review from rstam April 30, 2025 20:10
Copy link
Contributor

@rstam rstam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small changes suggested.


namespace MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToAggregationExpressionTranslators
{
internal static class NewDictionaryExpressionToAggregationExpressionTranslator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this line is indented too far.

{
public static bool CanTranslate(NewExpression expression)
=> expression.Type.IsConstructedGenericType &&
expression.Type.GetGenericTypeDefinition() == typeof(Dictionary<,>) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the checks in line 31-32 should actually be in IsWithIEnumerableKeyValuePairConstructor, not here.

You should be able to call DictionaryConstructor.IsWithIEnumerableKeyValuePairConstructor with ANY ConstructorInfo and it should return the correct answer.

@sanych-sun sanych-sun requested a review from rstam April 30, 2025 22:46
Copy link
Contributor

@rstam rstam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sanych-sun sanych-sun merged commit eff1f25 into mongodb:main Apr 30, 2025
19 of 23 checks passed
@sanych-sun sanych-sun deleted the csharp4779 branch April 30, 2025 23:19
@sanych-sun sanych-sun changed the title CSHARP-4779: Support Dictionary(IEnumerable<KeyValuePair<TKey, TValue>> collection) constructor in LINQ CSHARP-4779: Support Dictionary(IEnumerable<KeyValuePair<TKey, TValue>> collection) constructor in LINQ… May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants