CSHARP-5552: Add support for $convert in LINQ #1659

papafe · 2025-04-01T12:01:25Z

No description provided.

papafe · 2025-04-03T13:46:43Z

...ionExpressionTranslators/MethodTranslators/ConvertMethodToAggregationExpressionTranslator.cs

+            return (T)constantExpression.Value;
+        }
+
+        private static BsonType GetBsonType(Type type)  //TODO Do we have this kind of info somewhere else...?


Do we have this kind of mapping somewhere else in our code?

I can't find any.

There probably is nowhere else in the code where we need to map a C# type to a BsonType like this. Everywhere else it is up to the serializer to determine how the C# type is represented.

This is an unusual case because we decide to let the user express the C# return type using the TTo generic type parameter, and we let the C# type imply the BsonType.

rstam · 2025-04-07T01:36:25Z

src/MongoDB.Driver/Linq/Linq3Implementation/Ast/Expressions/AstExpression.cs

-                toValue != null &&
-                onError == null &&
-                onNull == null)
+            if (onError == null && onNull == null && subType == null && format == null && byteOrder == null &&


Consider formatting with one condition per line to make it easier to read:

if (to is AstConstantExpression toConstantExpression && (toConstantExpression.Value as BsonString)?.Value is { } toValue && subType == null && byteOrder == null && format == null && onError == null && onNull == null)

rstam · 2025-04-07T01:37:02Z

...ionToAggregationExpressionTranslators/ConstantExpressionToAggregationExpressionTranslator.cs

@@ -14,7 +14,6 @@
 */

 using System.Linq.Expressions;
-using MongoDB.Bson.IO;


Unnecessary diff.

I noticed it while I was looking at the file. It's a very small change, do we really need to open another PR to fix this?
This is valid also for the other small changes unrelated to the PR.

I'm not suggesting we need another PR for this... I'm just saying we shouldn't modify files that have nothing at all to do with this PR.

This file can be "cleaned" up some future day when we are doing some work that actually involves this file.

What is your current thinking on this unnecessary diff? Can it be removed from this PR?

...ssionToAggregationExpressionTranslators/MemberExpressionToAggregationExpressionTranslator.cs

rstam · 2025-04-07T01:38:55Z

src/MongoDB.Driver/Mql.cs

+    /// <summary>
+    /// Represents the options parameter for <see cref="Mql.Convert{TFrom, TTo}(TFrom, ConvertOptions{TTo})"/>.
+    /// </summary>
+    public abstract class ConvertOptions


This should probably be in a its own file called ConvertOptions.cs.

Along with ByteOrder (unless you think ByteOrder should be in its own file).

...ionExpressionTranslators/MethodTranslators/ConvertMethodToAggregationExpressionTranslator.cs

rstam · 2025-04-07T01:45:33Z

...ionExpressionTranslators/MethodTranslators/ConvertMethodToAggregationExpressionTranslator.cs

+            return new TranslatedExpression(expression, ast, serializer);
+        }
+
+        private static void ExtractOptionsFromConstantExpression(ConstantExpression constantExpression, out ByteOrder? byteOrder, out string format, out AstExpression onErrorAst, out AstExpression onNullAst, out BsonBinarySubType? subType)


See suggested TranslateOptions above.

rstam · 2025-04-07T01:45:43Z

...ionExpressionTranslators/MethodTranslators/ConvertMethodToAggregationExpressionTranslator.cs

+            byteOrder = options.ByteOrder;
+        }
+
+        private static void ExtractOptionsFromMemberInitExpression(MemberInitExpression memberInitExpression, TranslationContext context, out ByteOrder? byteOrder, out string format, out AstExpression onErrorAst, out AstExpression onNullAst, out BsonBinarySubType? subType)


See suggested TranslateOptions above.

rstam · 2025-04-07T01:46:16Z

...ionExpressionTranslators/MethodTranslators/ConvertMethodToAggregationExpressionTranslator.cs

+            }
+        }
+
+        private static T GetConstantValue<T>(Expression expression, string fieldName)


Use existing expression.GetConstantValue<TValue> instead.

rstam · 2025-04-07T01:47:11Z

...ionExpressionTranslators/MethodTranslators/ConvertMethodToAggregationExpressionTranslator.cs

+            return (T)constantExpression.Value;
+        }
+
+        private static BsonType GetBsonType(Type type)


Rename to TranslateToType and return two things,, the BsonType and the toSerializer.

private static (BsonType ToBsonType, IBsonSerializer ToSerialzier) TranslateToType(Expression expression, Type toType) { return Type.GetTypeCode(Nullable.GetUnderlyingType(toType) ?? toType) switch { TypeCode.Boolean => (BsonType.Boolean, BooleanSerializer.Instance), TypeCode.Byte => (BsonType.Int32, ByteSerializer.Instance), TypeCode.Char => (BsonType.String, StringSerializer.Instance), TypeCode.DateTime => (BsonType.DateTime, DateTimeSerializer.Instance), TypeCode.Decimal => (BsonType.Decimal128, DecimalSerializer.Instance), TypeCode.Double => (BsonType.Double, DoubleSerializer.Instance), TypeCode.Int16 => (BsonType.Int32, Int16Serializer.Instance), TypeCode.Int32 => (BsonType.Int32, Int32Serializer.Instance), TypeCode.Int64 => (BsonType.Int64, Int64Serializer.Instance), TypeCode.SByte => (BsonType.Int32, SByteSerializer.Instance), TypeCode.Single => (BsonType.Double, DoubleSerializer.Instance), TypeCode.String => (BsonType.String, StringSerializer.Instance), TypeCode.UInt16 => (BsonType.Int32, UInt16Serializer.Instance), TypeCode.UInt32 => (BsonType.Int64, Int32Serializer.Instance), TypeCode.UInt64 => (BsonType.Decimal128, UInt64Serializer.Instance), _ when toType == typeof(byte[]) => (BsonType.Binary, ByteArraySerializer.Instance), _ when toType == typeof(BsonBinaryData) => (BsonType.Binary, BsonBinaryDataSerializer.Instance), _ when toType == typeof(Decimal128) => (BsonType.Decimal128, Decimal128Serializer.Instance), _ when toType == typeof(Guid) => (BsonType.Binary, GuidSerializer.StandardInstance), _ when toType == typeof(ObjectId) => (BsonType.ObjectId, ObjectIdSerializer.Instance), _ => throw new ExpressionNotSupportedException(expression, because: $"{toType} is not a valid TTo for Convert" }; }

This will require adding a few missing XyzSerializer.Instance properties. Follow the existing pattern from serializers that already have an Instance property.

Wouldn't it be convenient here to use the serializers registered in StandardSerializers? It seems that that's the perfect class for what we're trying to do here, as it takes care also of nullable value types. Of course we'd need to specify the serializers that are not in that list.

src/MongoDB.Bson/Serialization/Serializers/ByteSerializer.cs

rstam · 2025-04-07T16:09:50Z

...ionToAggregationExpressionTranslators/ConstantExpressionToAggregationExpressionTranslator.cs

@@ -14,7 +14,6 @@
 */

 using System.Linq.Expressions;
-using MongoDB.Bson.IO;


I'm not suggesting we need another PR for this... I'm just saying we shouldn't modify files that have nothing at all to do with this PR.

This file can be "cleaned" up some future day when we are doing some work that actually involves this file.

...ionExpressionTranslators/MethodTranslators/ConvertMethodToAggregationExpressionTranslator.cs

rstam · 2025-04-07T16:12:30Z

src/MongoDB.Driver/ConvertOptions.cs

+        private bool _onErrorWasSet;
+        private TTo _onNull;
+        private bool _onNullWasSet;
+        private readonly IBsonSerializer _serializer;


Looks like you forgot to remove the serializer here.

src/MongoDB.Bson/Serialization/Serializers/ByteSerializer.cs

src/MongoDB.Bson/Serialization/Serializers/Int16Serializer.cs

...ionExpressionTranslators/MethodTranslators/ConvertMethodToAggregationExpressionTranslator.cs

rstam · 2025-04-07T18:09:14Z

...ionExpressionTranslators/MethodTranslators/ConvertMethodToAggregationExpressionTranslator.cs

+                _ => throw new ExpressionNotSupportedException(expression, because: $"{toType} is not a valid TTo for Convert")
+            };
+
+            return (bsonType, StandardSerializers.GetSerializer(toType));


I don't like the idea of using the StandardSerializers here.

We are working with a defined set of of toType serializers that may or may not be the same as the StandardSerializers. In fact they are not, as you discovered by having to handle some cases specially.

I liked your original switch where you handled all cases in one switch, just modified to return the serializer also:

private static (BsonType ToBsonType, IBsonSerializer ToSerialzier) TranslateToType(Expression expression, Type toType) { return Type.GetTypeCode(Nullable.GetUnderlyingType(toType) ?? toType) switch { TypeCode.Boolean => (BsonType.Boolean, BooleanSerializer.Instance), TypeCode.Byte => (BsonType.Int32, ByteSerializer.Instance), TypeCode.Char => (BsonType.String, StringSerializer.Instance), TypeCode.DateTime => (BsonType.DateTime, DateTimeSerializer.Instance), TypeCode.Decimal => (BsonType.Decimal128, DecimalSerializer.Instance), TypeCode.Double => (BsonType.Double, DoubleSerializer.Instance), TypeCode.Int16 => (BsonType.Int32, Int16Serializer.Instance), TypeCode.Int32 => (BsonType.Int32, Int32Serializer.Instance), TypeCode.Int64 => (BsonType.Int64, Int64Serializer.Instance), TypeCode.SByte => (BsonType.Int32, SByteSerializer.Instance), TypeCode.Single => (BsonType.Double, DoubleSerializer.Instance), TypeCode.String => (BsonType.String, StringSerializer.Instance), TypeCode.UInt16 => (BsonType.Int32, UInt16Serializer.Instance), TypeCode.UInt32 => (BsonType.Int64, Int32Serializer.Instance), TypeCode.UInt64 => (BsonType.Decimal128, UInt64Serializer.Instance), _ when toType == typeof(byte[]) => (BsonType.Binary, ByteArraySerializer.Instance), _ when toType == typeof(BsonBinaryData) => (BsonType.Binary, BsonBinaryDataSerializer.Instance), _ when toType == typeof(Decimal128) => (BsonType.Decimal128, Decimal128Serializer.Instance), _ when toType == typeof(Guid) => (BsonType.Binary, GuidSerializer.StandardInstance), _ when toType == typeof(ObjectId) => (BsonType.ObjectId, ObjectIdSerializer.Instance), _ => throw new ExpressionNotSupportedException(expression, because: $"{toType} is not a valid TTo for Convert") }; }

I'll go back to this, but we still need to take care of nullable types that were already registered in StandardSerializers. This also means that we'll need to create the nullable serializers here from scratch, as we don't have static instances we can use.

I forgot that TTo could be nullable. Does that change what you want to do?

My initial concern with using StandardSerializers was that I wasn't 100% sure what "Standard" meant and whether it would always result in the right thing being done (for Convert).

I was also concerned that some of the serializers needed by Convert might not belong in the "Standard" set.

It's true that some of the serializers need by convert are not in the "Standard" set, but it seems that it covered most of the "basic" types. It seems that StandardSerializers' methods are used by ConstantExpressionToAggregationExpressionTranslator to serialize the constant value, so I thought this could be a similar thing.
Anyways, I think the current solution makes sense, the only small annoyance is that we don't have static "Instances" of the nullable serializers, but I think it's a small thing.

src/MongoDB.Driver/ConvertOptions.cs

rstam · 2025-04-10T16:35:06Z

...ionExpressionTranslators/MethodTranslators/ConvertMethodToAggregationExpressionTranslator.cs

+        {
+            var underlyingType = Nullable.GetUnderlyingType(toType);
+            var isNullable = underlyingType != null;
+            var effectiveType = underlyingType ?? toType;


I very much dislike calling Nullable.GetUnderlyingType when we do NOT know if the type is nullable or not. It seems like this method should throw an exception when called with a non-nullable type. I know it does not, but that seems more like a quirk and an implementation error on Microsoft's part, one that we should not rely on.

I suggest:

var isNullable = toType.IsNullable(); var valueType = isNullable ? Nullable.GetUnderlyingType(toType) : toType;

effectiveType is not a terrible name, but we tend to use valueType when talking about nullable values.

rstam · 2025-04-10T16:40:31Z

...ionExpressionTranslators/MethodTranslators/ConvertMethodToAggregationExpressionTranslator.cs

+            var isNullable = underlyingType != null;
+            var effectiveType = underlyingType ?? toType;
+
+            (BsonType, IBsonSerializer) result = Type.GetTypeCode(effectiveType) switch


I was trying to figure out why you had to use this odd result variable here...

The reason is that the switch expression doesn't have a "best" result type that applies to all cases, and the compiler needs some guidance. The declaration of the result variable is one way to provide that guidance.

Consider this alternative that doesn't require the intermediate result variable:

var (bsonType, valueSerializer) = (ValueTuple<BsonType, IBsonSerializer>)(Type.GetTypeCode(valueType) switch { .... });

where casting the result of the switch expression to the ValueTuple provides the guidance to the compiler.

Alternatively, if you were to cast even ONE of the results to IBsonSerializer that would work also, but it is a bit esoteric to do it that way:

TypeCode.Boolean => (BsonType.Boolean, (IBsonSerializer)BooleanSerializer.Instance),

That's cool, I didn't think about it.

rstam · 2025-04-10T16:41:57Z

...ionToAggregationExpressionTranslators/ConstantExpressionToAggregationExpressionTranslator.cs

@@ -14,7 +14,6 @@
 */

 using System.Linq.Expressions;
-using MongoDB.Bson.IO;


What is your current thinking on this unnecessary diff? Can it be removed from this PR?

sanych-sun · 2025-04-10T17:53:31Z

src/MongoDB.Driver/ConvertOptions.cs

+            set => _subType = value;
+        }
+
+        internal abstract bool OnErrorWasSet(out object onError);


Should it be named TryGetOnError?

I think it would make sense.

sanych-sun

Please check evergreen also. There are some test failures related to the changes.

rstam · 2025-04-11T00:00:20Z

src/MongoDB.Driver/ConvertOptions.cs

+
+        internal abstract bool TryGetOnError(out object onError);
+
+        internal abstract bool TryGetOnNull(out object onNull);


I'm not convinced this name change is a good one.

Not every method that returns a bool should be named TryXyz.

In my opinion methods should only be named TryXyz when they have a chance of failure, but we want to return false instead of throwing an exception.

In this case there is no chance of failure. We merely want to know whether OnError or OnNull was set, and if so to what.

papafe added 30 commits March 24, 2025 15:50

Small correction

b11abcb

Improvements

e51d733

Fixed errors

7484967

Small rename

d604e2a

Various corrections

e46ebc6

Behaviour correction

9ff2347

Simplified code

3d55596

Small improvements

ebe1bf5

Added test

20763ce

Several improvements

64b0082

Added int tests

be829c9

Added convertToLong tests

4007671

Small corrections

9b40d91

Various fixes

9e6c11a

Small corrections

f1cc942

Fixed docs

6b3fb54

Fixed docs

f25e15b

Added byteOrder, removed format where not necessary

73fe700

Small fix

94651f8

Imrpoved tests

82b3f02

Added tests

6bfc029

Fixed test

23556b3

Various fixes

76dd221

Naming correction

6b6be70

Small naming correction

311832d

Small corrections

34ef476

Fixes according to PR

5f1732f

Moved byteOrder to outer scope

b17c354

Changed order

37ab91e

Improved testing

54395c1

papafe added 2 commits April 3, 2025 15:08

Small rename

9b88903

Reordered tests

5fa5850

papafe commented Apr 3, 2025

View reviewed changes

Slight improvements

03c3974

papafe requested a review from rstam April 3, 2025 13:52

papafe marked this pull request as ready for review April 3, 2025 16:11

papafe requested a review from a team as a code owner April 3, 2025 16:11

papafe added 5 commits April 4, 2025 11:24

Small fix

9aeabb3

Removed constraints

07de4e5

Tried to add new tests

39904d0

Added generic convert test

e4fc2a7

Small fix

dc2841e

rstam requested changes Apr 7, 2025

View reviewed changes

papafe added 4 commits April 7, 2025 10:12

Small fixes

276434d

Fixes according to PR

23de869

Corrections

d844cd2

Small corrections

ec52d7f

papafe commented Apr 7, 2025

View reviewed changes

src/MongoDB.Bson/Serialization/Serializers/ByteSerializer.cs Show resolved Hide resolved

rstam requested changes Apr 7, 2025

View reviewed changes

papafe added 4 commits April 8, 2025 09:33

Added test

f8a5a66

Removed unused

abda353

Restored files

e2f2f6a

Small fixes

afa7bdd

papafe requested a review from rstam April 8, 2025 15:58

rstam requested changes Apr 10, 2025

View reviewed changes

sanych-sun requested changes Apr 10, 2025

View reviewed changes

sanych-sun self-requested a review April 10, 2025 19:02

sanych-sun requested changes Apr 10, 2025

View reviewed changes

Small corrections + test fix

de05dd6

rstam requested changes Apr 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSHARP-5552: Add support for $convert in LINQ #1659

CSHARP-5552: Add support for $convert in LINQ #1659

papafe commented Apr 1, 2025

papafe Apr 3, 2025

rstam Apr 3, 2025

rstam Apr 7, 2025

papafe Apr 7, 2025

rstam Apr 7, 2025

papafe Apr 7, 2025 •

edited

Loading

rstam Apr 7, 2025

rstam Apr 10, 2025

rstam Apr 7, 2025

rstam Apr 7, 2025

rstam Apr 7, 2025

rstam Apr 7, 2025

rstam Apr 7, 2025

papafe Apr 7, 2025 •

edited

Loading

rstam Apr 7, 2025

rstam Apr 7, 2025

rstam Apr 7, 2025

papafe Apr 8, 2025 •

edited

Loading

rstam Apr 8, 2025

papafe Apr 8, 2025

rstam Apr 10, 2025

rstam Apr 10, 2025

papafe Apr 10, 2025

rstam Apr 10, 2025

sanych-sun Apr 10, 2025

papafe Apr 10, 2025

sanych-sun left a comment

rstam Apr 11, 2025


		internal abstract bool TryGetOnError(out object onError);

		internal abstract bool TryGetOnNull(out object onNull);

CSHARP-5552: Add support for $convert in LINQ #1659

Are you sure you want to change the base?

CSHARP-5552: Add support for $convert in LINQ #1659

Conversation

papafe commented Apr 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe Apr 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanych-sun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe Apr 7, 2025 •

edited

Loading

papafe Apr 7, 2025 •

edited

Loading

papafe Apr 8, 2025 •

edited

Loading