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

Jump to bottom

Merged

sanych-sun merged 4 commits into mongodb:main from sanych-sun:csharp4779

Apr 30, 2025

Merged

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

sanych-sun merged 4 commits into mongodb:main from sanych-sun:csharp4779

Apr 30, 2025

Conversation

@sanych-sun

Copy link

Member

@sanych-sun sanych-sun commented Apr 1, 2025

No description provided.

@sanych-sun sanych-sun added the feature label

Apr 1, 2025

@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 adelinowona, papafe and rstam and removed request for a team and papafe

April 1, 2025 00:27

rstam

rstam reviewed

Apr 1, 2025

View reviewed changes

...gationExpressionTranslators/NewDictionaryExpressionToAggregationExpressionTranslatorTests.cs Outdated

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

@sanych-sun sanych-sun Apr 1, 2025

Choose a reason for hiding this comment

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

Done

adelinowona

adelinowona requested changes

Apr 1, 2025

View reviewed changes

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.

...gationExpressionTranslators/NewDictionaryExpressionToAggregationExpressionTranslatorTests.cs Outdated

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

@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.

forgot to remove this?

Copy link

Member Author

@sanych-sun sanych-sun Apr 1, 2025

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

@sanych-sun sanych-sun Apr 7, 2025

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.

...AggregationExpressionTranslators/NewDictionaryExpressionToAggregationExpressionTranslator.cs Outdated

Comment on lines 79 to 80

if (keySerializer is not IRepresentationConfigurable representationConfigurableSerializer

|| representationConfigurableSerializer.Representation != BsonType.String)

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.

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

Copy link

Member Author

@sanych-sun sanych-sun Apr 1, 2025

Choose a reason for hiding this comment

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

Done

...AggregationExpressionTranslators/NewDictionaryExpressionToAggregationExpressionTranslator.cs Outdated

Comment on lines 44 to 45

if (itemSerializationInfo.Serializer is IRepresentationConfigurable representationConfigurable &&

representationConfigurable.Representation == BsonType.Array)

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.

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

Copy link

Member Author

@sanych-sun sanych-sun Apr 1, 2025

Choose a reason for hiding this comment

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

Done

...AggregationExpressionTranslators/NewDictionaryExpressionToAggregationExpressionTranslator.cs

var collectionExpression = arguments.Single();

var collectionTranslation = ExpressionToAggregationExpressionTranslator.TranslateEnumerable(context, collectionExpression);

AstExpression collectionTranslationAst;

Copy link

Contributor

@adelinowona adelinowona Apr 1, 2025 •

edited

Loading

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

@sanych-sun sanych-sun Apr 1, 2025

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

@sanych-sun


 CSHARP-4779: Support Dictionary(IEnumerable<KeyValuePair<TKey, TValue...

4b67387

...>> collection) constructor in LINQ3

@sanych-sun sanych-sun force-pushed the csharp4779 branch from dc144dc to 4b67387 Compare

April 3, 2025 18:13

@sanych-sun

PR

f1a774a

adelinowona

adelinowona approved these changes

Apr 24, 2025

View reviewed changes

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

rstam

rstam requested changes

Apr 28, 2025

View reviewed changes

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

PR

fca3c4f

@sanych-sun sanych-sun requested a review from rstam

April 30, 2025 20:10

rstam

rstam requested changes

Apr 30, 2025

View reviewed changes

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.

...AggregationExpressionTranslators/NewDictionaryExpressionToAggregationExpressionTranslator.cs Outdated

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.

...AggregationExpressionTranslators/NewDictionaryExpressionToAggregationExpressionTranslator.cs Outdated

{

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

PR

30bb7a4

@sanych-sun sanych-sun requested a review from rstam

April 30, 2025 22:46

rstam

rstam approved these changes

Apr 30, 2025

View reviewed changes

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

Labels

feature

3 participants

@sanych-sun @rstam @adelinowona

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

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

Uh oh!

Conversation

@sanych-sun sanych-sun commented Apr 1, 2025

Uh oh!

@rstam rstam Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

@sanych-sun sanych-sun Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

@adelinowona adelinowona left a comment

Choose a reason for hiding this comment

Uh oh!

@adelinowona adelinowona Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

@sanych-sun sanych-sun Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

@sanych-sun sanych-sun Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

@adelinowona adelinowona Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

@sanych-sun sanych-sun Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

@adelinowona adelinowona Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

@sanych-sun sanych-sun Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

@adelinowona adelinowona Apr 1, 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!

@sanych-sun sanych-sun Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

@adelinowona adelinowona left a comment

Choose a reason for hiding this comment

Uh oh!

@rstam rstam left a comment

Choose a reason for hiding this comment

Uh oh!

@rstam rstam left a comment

Choose a reason for hiding this comment

Uh oh!

@rstam rstam Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

@rstam rstam Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

@rstam rstam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

@adelinowona adelinowona Apr 1, 2025 •

edited

Loading