-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CSHARP-1913: Support using Dictionary fields as IEnumerable<KeyValuePair<TKey, TValue>> #1800
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
base: main
Are you sure you want to change the base?
Conversation
public bool TryGetItemSerializationInfo(out BsonSerializationInfo serializationInfo) | ||
{ | ||
if (_dictionaryRepresentation != DictionaryRepresentation.ArrayOfDocuments) | ||
if (_dictionaryRepresentation is DictionaryRepresentation.ArrayOfArrays or DictionaryRepresentation.ArrayOfDocuments) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were sort of working with ArrayOfDocuments representation, but this adds support for ArrayOfArrays.
var ienumerableSerializer = ArraySerializerHelper.CreateSerializer(keyValuePairSerializer); | ||
|
||
return new TranslatedExpression(aggregateExpression.Expression, ast, enumerableSerializer); | ||
aggregateExpression = new TranslatedExpression(expression, ast, ienumerableSerializer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of the above cases share this in common: they are both cases where an enumerable value has the values represented in some unusual way other than as a BsonArray
.
The first case corresponds to IGrouping<TKey, TElement>
where the elements are wrapped one level deeper in an _elements
field.
The second is new in this PR, and corresponds to a Dictionary<TKey, TValue>
with the Document
representation. In order to use this dictionary as an enumerable of KeyValuePair<TKey, TValue>
we have to insert a call to $objectToArray
.
var selectorParameter = selectorLambda.Parameters.Single(); | ||
var selectorParameterSymbol = context.CreateSymbol(selectorParameter, sourceSerializer, isCurrent: true); | ||
var selectorContext = context.WithSymbol(selectorParameterSymbol); | ||
var selectorTranslation = ExpressionToAggregationExpressionTranslator.TranslateEnumerable(selectorContext, selectorLambda.Body); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to call TranslateEnumerable
instead of Translate
.
AssertStages( | ||
stages, | ||
"{ $match : { Name : 'TestName' } }", | ||
"{ $project : { _v : { $reduce : { input : { $map : { input : '$DictionaryWithArrayOfArraysRepresentation', as : 'kvp', in : ['$$kvp'] } }, initialValue : [], in : { $concatArrays : ['$$value', '$$this'] } } }, _id : 0 } }"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have liked to use kvp.Key
or kvp.Value
in the test test but that is not (yet) supported when the KeyValuePair
is represented as an array. So I just used the entire KeyValuePair
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have getting the Key
or Value
of a KeyValuePair
represented as an array supported in my work for full support of dictionary representations. So I'll probably update this test when I rebase on this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in our meeting, we can remove this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test serves a useful purpose in proving that this PR also fixes 4251 but at your request I will remove it.
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra spacing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
AssertStages( | ||
stages, | ||
"{ $match : { Name : 'TestName' } }", | ||
"{ $project : { _v : { $reduce : { input : { $map : { input : '$DictionaryWithArrayOfArraysRepresentation', as : 'kvp', in : ['$$kvp'] } }, initialValue : [], in : { $concatArrays : ['$$value', '$$this'] } } }, _id : 0 } }"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have getting the Key
or Value
of a KeyValuePair
represented as an array supported in my work for full support of dictionary representations. So I'll probably update this test when I rebase on this PR.
SelectMany expects the selector to return an enumerable value.
Dictionary is a special case of an enumerable value, and we have to treat it specially.