CSHARP-5515: Implement $convert binData <-> int, long #1637
Conversation
|
@rstam This is a draft, as there are a couple of things missing, but I wanted to get some initial feedback. |
| { | ||
| throw CustomLinqExtensionMethodHelper.CreateNotSupportedException(); | ||
| } | ||
|
|
There was a problem hiding this comment.
By the way, in the comment before the previous one (with the 3 scenarios), the Optional<T> class was designed precisely to tell the difference between an explicit null value and not passing a value at all.
When you have a method like foo(int? arg = null) you can't tell the difference between the following two:
foo(); // arg omitted and defaults to `null`
foo(null); // explicit null provided
But it the method is declared foo(Optional<int?> arg = default(Optional<int?>)) you can:
foo(); // arg omitted and defaults to `default(Optional<int?>)` (which is not the same as `null`)
foo(null); // explicit null provided
But... Boris doesn't like Optional<T> so that might be a non-starter.
src/MongoDB.Driver/Linq/Linq3Implementation/Ast/Expressions/AstConvertExpression.cs
Show resolved
Hide resolved
src/MongoDB.Driver/Linq/Linq3Implementation/Ast/Expressions/AstExpression.cs
Outdated
Show resolved
Hide resolved
src/MongoDB.Driver/Mql.cs
Outdated
| /// <param name="subtype">The BsonBinaryData subtype of the result value.</param> | ||
| /// <param name="format">The format string.</param> | ||
| /// <returns>The converted value.</returns> | ||
| public static BsonBinaryData ToBinData(string value, BsonBinarySubType subtype, string format) |
There was a problem hiding this comment.
I've been calling this method ToBsonBinaryData but you keep using ToBinData.
I understand it's shorter, but it's not accurate. There is no data type called BinData.
src/MongoDB.Driver/Mql.cs
Outdated
| /// <param name="subtype">The BsonBinaryData subtype of the result value.</param> | ||
| /// <param name="byteOrder">The byte order of BsonBinaryData.</param> | ||
| /// <returns>The converted value.</returns> | ||
| public static BsonBinaryData ToBinData(int? value, BsonBinarySubType subtype, ByteOrder byteOrder) |
There was a problem hiding this comment.
We need separate overloads for int and int?.
A property in a user's POCO might be an int or it might be an int?.
I realize there is an automatic conversion from int to int? but I don't know if that's how we would want to handle that.
| /// <param name="value">The value.</param> | ||
| /// <param name="byteOrder">The byte ordering of BsonBinaryData.</param> | ||
| /// <returns>The converted value.</returns> | ||
| public static int? ToNullableInt(BsonBinaryData value, ByteOrder byteOrder) |
There was a problem hiding this comment.
We need separate ToInt32 and ToNullableInt32 methods so that the user can control the type of the result.
They might not want the result to be nullable.
| { | ||
| throw CustomLinqExtensionMethodHelper.CreateNotSupportedException(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Ferdinando and I discussed in Slack an approach with an options class that would allow the user to write something like the following:
Mql.ToInt(x.BinaryField, ByteOrder.LittleEndian))
Mql.ToInt(x.BinaryField, ByteOrder.LittleEndian, new() { OnError. = 0 })
Mql.ToInt(x.BinaryField, ByteOrder.LittleEndian, new() { OnNull = 0 })
Mql.ToInt(x.BinaryField, ByteOrder.LittleEndian, new() { OnError = 0, OnNull = 0 }))
This would require only two overloads. One with options and one without.
The options class can keep track whether OnError or OnNull were set or not.
| public BsonBinaryData BinaryProperty { get; set; } | ||
| public double? DoubleProperty { get; set; } | ||
| public int? IntProperty { get; set; } | ||
| public long? LongProperty { get; set; } |
There was a problem hiding this comment.
These properties are named incorrectly because they are nullable. They should be named:
public double? NullableDoubleProperty { get; set; }
public int? NullableIntProperty { get; set; }
public long? NullableLongProperty { get; set; }
At the same time, we need to add the following three NON-nullable properties to test against:
public double DoubleProperty { get; set; }
public int IntProperty { get; set; }
public long LongProperty { get; set; }
| @@ -44,6 +44,8 @@ public class Feature | |||
| private static readonly Feature __clientBulkWrite = new Feature("ClientBulkWrite", WireVersion.Server80); | |||
| private static readonly Feature __clientSideEncryption = new Feature("ClientSideEncryption", WireVersion.Server42); | |||
| private static readonly Feature __clusteredIndexes = new Feature("ClusteredIndexes", WireVersion.Server53); | |||
| private static readonly Feature __convertOperatorBinDataToFromNumeric = new Feature("ConvertBinDataToFromNumeric", WireVersion.Server81); | |||
| private static readonly Feature __convertOperatorBinDataToFromString= new Feature("ConvertBinDataToFromString", WireVersion.Server80); | |||
There was a problem hiding this comment.
Nit: change string constants to match feature name.
| private readonly AstExpression _onError; | ||
| private readonly AstExpression _onNull; | ||
| private readonly string _format; | ||
| private readonly ConvertOptions _options; |
There was a problem hiding this comment.
Undo all these changes at the AST level.
These changes are not necessary and the AST level should be self contained and not depend on any classes like ConvertOptions defined at a higher level in the public API.
There was a problem hiding this comment.
I see your point, I restored it to the older version. I wasn't sure of at what level we wanted to make the conversion.
No description provided.