CSHARP-5375: Remove KnownSerializerRegistry. #1552

rstam · 2024-11-20T22:23:43Z

No description provided.

rstam · 2024-11-20T22:24:59Z

src/MongoDB.Driver/Linq/Linq3Implementation/Misc/PartialEvaluator.cs

            typeof(DateTimeExtensions),
            typeof(LinqExtensions),
            typeof(MongoEnumerable),
+            typeof(Mql),


To keep Mql.Constant(...) from being evaluated client-side.

rstam · 2024-11-20T22:26:16Z

...nExpressionTranslators/MethodTranslators/AggregateMethodToAggregationExpressionTranslator.cs

                    var funcParameters = funcLambda.Parameters;
                    var accumulatorParameter = funcParameters[0];
-                    var accumulatorSerializer = context.KnownSerializersRegistry.GetSerializer(accumulatorParameter);
+                    var accumulatorSerializer = BsonSerializer.LookupSerializer(accumulatorParameter.Type);


It might make sense to have a factory for numeric return types to protect ourselves from users registering weird serializers for numeric types.

Actually the previous comment is moot. What we should be doing is using the serializer from the seedTranslation as the serializer for the accumulator.

rstam · 2024-11-20T22:26:52Z

...onExpressionTranslators/MethodTranslators/ConstantMethodToAggregationExpressionTranslator.cs

+                    }
+                    else
+                    {
+                        throw new ExpressionNotSupportedException(expression, because: "the registred serializer is not representation configurable");


Oops, contains a typo... will fix in next commit.

rstam · 2024-11-20T22:27:41Z

...ToAggregationExpressionTranslators/ConditionalExpressionToAggregationExpressionTranslator.cs

+                AggregationExpression ifTrueTranslation;
+                AggregationExpression ifFalseTranslation;
+                IBsonSerializer resultSerializer;
+                if (ifTrueExpression is ConstantExpression ifTrueConstantExpression)


If one result is a constant use the serializer of the other expression to serialize the constant.

rstam · 2024-11-20T22:28:17Z

...ToAggregationExpressionTranslators/NewDateTimeExpressionToAggregationExpressionTranslator.cs


                var ast = AstExpression.DateFromParts(yearTranslation.Ast, monthTranslation.Ast, dayTranslation.Ast, hourTranslation?.Ast, minuteTranslation?.Ast, secondTranslation?.Ast, millisecondTranslation?.Ast);
-                var serializer = context.KnownSerializersRegistry.GetSerializer(expression);
+                var serializer = DateTimeSerializer.Instance;


No need to look up anything. We know which serializer we need.

rstam · 2024-11-20T22:28:29Z

...ExpressionToFilterTranslators/ExpressionTranslators/ParameterExpressionToFilterTranslator.cs

                if (context.SymbolTable.TryGetSymbol(expression, out var symbol))
                {
-                    var serializer = context.KnownSerializersRegistry.GetSerializer(expression);
+                    var serializer = symbol.Serializer;


No need to look up anything. We know which serializer we need.

rstam · 2024-11-20T22:29:54Z

src/MongoDB.Driver/Linq/Linq3Implementation/Translators/TranslationContext.cs


        // public properties
        public TranslationContextData Data => _data;
-        public KnownSerializersRegistry KnownSerializersRegistry => _knownSerializersRegistry;


What do you think of making the serializer registry part of the context so we're not tied to using the global registry?

In the short term it's the same thing, but it looks towards the future where we want to specify different registries for different collections.

I like this idea. Up to you whether you want to include it in this PR or create a separate one.

I'm going to postpone doing anything like this. I don't even want to create a ticket for it until we know we need it.

By the way, I've been able to eliminate even more references to the serializer registry in the latest commit.

rstam · 2024-11-20T22:30:58Z

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

            (d => new R<E> { N = d.Id, V = d.I1 == E.E1 ? E.E1 : d.S2 }, "{ $project : { N : '$_id', V : { $cond : { if : { $eq : ['$I1', 1] }, then : 'E1', else : '$S2' } }, _id : 0 } }", new[] { E.E1, E.E2 }),
            (d => new R<E> { N = d.Id, V = d.I1 == E.E1 ? d.S1 : d.S2 }, "{ $project : { N : '$_id', V : { $cond : { if : { $eq : ['$I1', 1] }, then : '$S1', else : '$S2' } }, _id : 0 } }", new[] { E.E1, E.E2 }),
-            (d => new R<E> { N = d.Id, V = d.S1 == E.E1 ? E.E1 : E.E2 }, "{ $project : { N : '$_id', V : { $cond : { if : { $eq : ['$S1', 'E1'] }, then : 'E1', else : 'E2' } }, _id : 0 } }", new[] { E.E1, E.E2 }),
+            (d => new R<E> { N = d.Id, V = d.S1 == E.E1 ? Mql.Constant(E.E1, BsonType.String) : E.E2 }, "{ $project : { N : '$_id', V : { $cond : { if : { $eq : ['$S1', 'E1'] }, then : 'E1', else : 'E2' } }, _id : 0 } }", new[] { E.E1, E.E2 }),


This is the scenario we were discussing in Slack.

Since the ternary operator has two constants we now default to using the registered serializer, but using Mql.Constant the user can override that.

JamesKovacs

Overall looks great and allows us to remove a difficult-to-reason-about piece of code. One question around the TranslationContext and the now unused serializer parameter.

JamesKovacs · 2024-11-23T00:41:51Z

src/MongoDB.Driver/Linq/Linq3Implementation/Translators/TranslationContext.cs

            }
-            var knownSerializersRegistry = KnownSerializerFinder.FindKnownSerializers(expression, serializer);
-            return new TranslationContext(symbolTable, nameGenerator, knownSerializersRegistry, translationOptions, data);
+            return new TranslationContext(symbolTable, nameGenerator, translationOptions, data);


serializer is never used. Are we still translating $setWindowFields correctly?

Good catch. I've removed the unused serializer paramater.

$setWindowFields translators weren't using the KnownSerializerRegisty so they should continue to be translated correctly.

JamesKovacs · 2024-11-23T00:42:57Z

src/MongoDB.Driver/Linq/Linq3Implementation/Translators/TranslationContext.cs


        // public properties
        public TranslationContextData Data => _data;
-        public KnownSerializersRegistry KnownSerializersRegistry => _knownSerializersRegistry;


I like this idea. Up to you whether you want to include it in this PR or create a separate one.

sanych-sun · 2024-11-25T17:21:11Z

src/MongoDB.Driver/Mql.cs

+        /// <returns>The value.</returns>
+        public static TValue Constant<TValue>(TValue value, IBsonSerializer<TValue> serializer)
+        {
+            throw new NotSupportedException("This method is not functional. It is only usable in MongoDB LINQ queries.");


Haven't we moved this to helper method?

Yes... in a different PR that was not yet pushed to main when this code was written.

I will change this now that that PR has been pushed to main.

In order to do this I'm rebasing the PR on main.

rstam

Addressed all comments.

Probably needs at least one more careful pass before we decide it's ready to merge (for example the TODO comment).

rstam · 2024-11-25T23:54:13Z

...nExpressionTranslators/MethodTranslators/AggregateMethodToAggregationExpressionTranslator.cs

                    var funcParameters = funcLambda.Parameters;
                    var accumulatorParameter = funcParameters[0];
-                    var accumulatorSerializer = context.KnownSerializersRegistry.GetSerializer(accumulatorParameter);
+                    var accumulatorSerializer = BsonSerializer.LookupSerializer(accumulatorParameter.Type);


Actually the previous comment is moot. What we should be doing is using the serializer from the seedTranslation as the serializer for the accumulator.

rstam · 2024-11-25T23:54:24Z

...onExpressionTranslators/MethodTranslators/ConstantMethodToAggregationExpressionTranslator.cs

+                    }
+                    else
+                    {
+                        throw new ExpressionNotSupportedException(expression, because: "the registred serializer is not representation configurable");


rstam · 2024-11-25T23:55:39Z

src/MongoDB.Driver/Linq/Linq3Implementation/Translators/TranslationContext.cs


        // public properties
        public TranslationContextData Data => _data;
-        public KnownSerializersRegistry KnownSerializersRegistry => _knownSerializersRegistry;


I'm going to postpone doing anything like this. I don't even want to create a ticket for it until we know we need it.

By the way, I've been able to eliminate even more references to the serializer registry in the latest commit.

rstam · 2024-11-25T23:56:26Z

src/MongoDB.Driver/Mql.cs

+        /// <returns>The value.</returns>
+        public static TValue Constant<TValue>(TValue value, IBsonSerializer<TValue> serializer)
+        {
+            throw new NotSupportedException("This method is not functional. It is only usable in MongoDB LINQ queries.");


In order to do this I'm rebasing the PR on main.

rstam · 2024-11-26T17:43:02Z

src/MongoDB.Driver/Linq/Linq3Implementation/GroupingWithOutputExpressionStageDefinitions.cs

        {
            var partiallyEvaluatedOutput = (Expression<Func<TGrouping, TOutput>>)PartialEvaluator.EvaluatePartially(_output);
-            var context = TranslationContext.Create(partiallyEvaluatedOutput, inputSerializer, translationOptions);
+            var context = TranslationContext.Create(partiallyEvaluatedOutput, translationOptions);


Create method no longer has a serializer parameter.

rstam · 2024-11-26T17:43:34Z

...ssionToAggregationExpressionTranslators/BinaryExpressionToAggregationExpressionTranslator.cs

                Type { IsConstructedGenericType: true } t when t.GetGenericTypeDefinition() == typeof(Nullable<>) => (IBsonSerializer)Activator.CreateInstance(typeof(NullableSerializer<>).MakeGenericType(t.GenericTypeArguments[0])),
                Type { IsArray: true } t => (IBsonSerializer)Activator.CreateInstance(typeof(ArraySerializer<>).MakeGenericType(t.GetElementType())),
-                _ => context.KnownSerializersRegistry.GetSerializer(expression) // Required for Coalesce
+                _ => BsonSerializer.LookupSerializer(expression.Type) // Required for Coalesce


TODO: look into this a bit more and confirm whether a lookup is justified.

rstam · 2024-11-26T17:44:05Z

...nExpressionTranslators/MethodTranslators/AggregateMethodToAggregationExpressionTranslator.cs

                    var funcParameters = funcLambda.Parameters;
                    var accumulatorParameter = funcParameters[0];
-                    var accumulatorSerializer = context.KnownSerializersRegistry.GetSerializer(accumulatorParameter);
+                    var accumulatorSerializer = seedTranslation.Serializer;


No lookup is needed. We know which serializer to use.

rstam · 2024-11-26T17:44:21Z

...nExpressionTranslators/MethodTranslators/AggregateMethodToAggregationExpressionTranslator.cs

+                            var: AstExpression.VarBinding(resultSelectorAccumulatorSymbol.Var, ast),
                            @in: resultSelectorTranslation.Ast);
-                        serializer = context.KnownSerializersRegistry.GetSerializer(resultSelectorLambda);
+                        serializer = resultSelectorTranslation.Serializer;


No lookup is needed. We know which serializer to use.

rstam · 2024-11-26T17:44:35Z

...nExpressionTranslators/MethodTranslators/AggregateMethodToAggregationExpressionTranslator.cs

-                        var resultSelectorSymbol = context.CreateSymbol(resultSelectorParameter, resultSelectorParameterSerializer);
-                        var resultSelectorContext = context.WithSymbol(resultSelectorSymbol);
+                        var resultSelectorAccumulatorParameter = resultSelectorLambda.Parameters[0];
+                        var resultSelectorAccumulatorSymbol = context.CreateSymbol(resultSelectorAccumulatorParameter, accumulatorSerializer);


No lookup is needed. We know which serializer to use.

rstam · 2024-11-26T17:45:32Z

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

                    };

-                    var actualTypeSerializer = context.KnownSerializersRegistry.GetSerializer(sourceExpression);
+                    var actualTypeSerializer = BsonSerializer.LookupSerializer(actualType);


In the future we might add some way to ask a serializer which serializer to use for a derived type.

For now a lookup is the only option available.

sanych-sun

LGTM

sanych-sun

LGTM + minor suggestion

sanych-sun · 2024-12-09T21:01:20Z

src/MongoDB.Driver/Linq/Linq3Implementation/Serializers/StandardSerializers.cs

+
+            if (type.IsNullable(out var valueType) && TryGetSerializer(valueType, out var valueSerializer))
+            {
+                serializer = NullableSerializer.Create(valueSerializer);


Does it makes sense to use singletons for Nullable<> in the same way as we have for other standard serializers?

Made a number of changes to this class based on discussions in Slack.

sanych-sun · 2024-12-11T22:18:30Z

src/MongoDB.Driver/Linq/Linq3Implementation/Serializers/StandardSerializers.cs

+            __standardSerializers.Add(typeof(uint), new UInt32Serializer(representation: BsonType.Int32, strictConverter));
+            __standardSerializers.Add(typeof(ulong), new UInt64Serializer(representation: BsonType.Int64, strictConverter));
+
+            var standardTypes = __standardSerializers.Keys.ToArray(); // call ToArray to make a copy of the current Keys


What if we implement local method RegisterSerializer instead, like:

void RegisterSerializer(IBsonSerializer serializer) { __standardSerializers.Add(serializer.ValueType, serializer); var nullableSerializer = NullableSerializer.Create(serializer); __standardSerializers.Add(nullableSerializer.ValueType, nullableSerializer); var arraySerializer = ArraySerializerHelper.CreateSerializer(serializer); __standardSerializers.Add(arraySerializer.ValueType, nullableSerializer); var arrayOfNullableSerializer = ArraySerializerHelper.CreateSerializer(itemSerializer: nullableSerializer); __standardSerializers.Add(arrayOfNullableSerializer.ValueType, arrayOfNullableSerializer); }

As far as I understood serializer.ValueType - this is the exact type that serializer will process, so we do not have to construct it here, but can relay on what serializer says.
And then simply call it for each serializer, like:

RegisterSerializer(new BooleanSerializer(representation: BsonType.Boolean)); RegisterSerializer(new ByteSerializer(representation: BsonType.Int32)); RegisterSerializer(new CharSerializer(representation: BsonType.Int32)); RegisterSerializer(new DecimalSerializer(representation: BsonType.Decimal128, allowTruncationConverter)); RegisterSerializer(new DoubleSerializer(representation: BsonType.Double, strictConverter)); RegisterSerializer(new Int16Serializer(representation: BsonType.Int32, strictConverter)); RegisterSerializer(new Int32Serializer(representation: BsonType.Int32, strictConverter)); RegisterSerializer(new Int64Serializer(representation: BsonType.Int64, strictConverter)); RegisterSerializer(new SByteSerializer(representation: BsonType.Int32)); RegisterSerializer(new SingleSerializer(representation: BsonType.Double, allowTruncationConverter)); RegisterSerializer(new UInt16Serializer(representation: BsonType.Int32, strictConverter)); RegisterSerializer(new UInt32Serializer(representation: BsonType.Int32, strictConverter)); RegisterSerializer(new UInt64Serializer(representation: BsonType.Int64, strictConverter));

I like it! Done.

Only difference is I named the local method AddStandardSerializers instead.

sanych-sun

LGTM, but please check the failed tests.

JamesKovacs

LGTM

sanych-sun

LGTM

rstam requested review from JamesKovacs and sanych-sun November 20, 2024 22:23

rstam requested a review from a team as a code owner November 20, 2024 22:23

rstam commented Nov 20, 2024

View reviewed changes

rstam force-pushed the csharp5375 branch from e9695f8 to c590f96 Compare November 21, 2024 16:56

JamesKovacs reviewed Nov 23, 2024

View reviewed changes

sanych-sun requested changes Nov 25, 2024

View reviewed changes

rstam force-pushed the csharp5375 branch from c590f96 to 8c37aea Compare November 26, 2024 00:36

rstam commented Nov 26, 2024

View reviewed changes

rstam requested review from JamesKovacs and sanych-sun November 26, 2024 17:47

sanych-sun approved these changes Nov 29, 2024

View reviewed changes

rstam force-pushed the csharp5375 branch from 09c69bd to 7d8e124 Compare December 4, 2024 20:15

rstam requested a review from sanych-sun December 4, 2024 20:16

sanych-sun approved these changes Dec 9, 2024

View reviewed changes

rstam requested a review from sanych-sun December 11, 2024 04:01

sanych-sun reviewed Dec 11, 2024

View reviewed changes

rstam requested a review from sanych-sun December 11, 2024 23:23

sanych-sun approved these changes Dec 12, 2024

View reviewed changes

rstam requested a review from sanych-sun December 13, 2024 16:45

JamesKovacs approved these changes Dec 19, 2024

View reviewed changes

rstam requested a review from adelinowona December 19, 2024 16:20

sanych-sun approved these changes Dec 19, 2024

View reviewed changes

CSHARP-5375: Remove KnownSerializerRegistry.

1d51d7b

rstam force-pushed the csharp5375 branch from ce82dc2 to 1d51d7b Compare December 20, 2024 20:56

rstam merged commit 1d51d7b into mongodb:main Dec 20, 2024
1 check was pending

CSHARP-5375: Remove KnownSerializerRegistry. #1552

CSHARP-5375: Remove KnownSerializerRegistry. #1552

Uh oh!

Conversation

rstam commented Nov 20, 2024

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!

JamesKovacs 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!

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!

sanych-sun left a comment

Choose a reason for hiding this comment

Uh oh!

sanych-sun 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!

sanych-sun left a comment •

edited

Loading