AVRO-2825: [csharp] Resolve: C# Logical Types throw exception on unkn…#2751
AVRO-2825: [csharp] Resolve: C# Logical Types throw exception on unkn…#2751TomBruns wants to merge 8 commits intoapache:mainfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
resolved all identified issues in new PR |
|
We need a C# developer/user to review the changes. |
|
Is there something I need to do to allow this PR to move forward? |
| /// </summary> | ||
| /// <param name="nullible">A flag indicating whether it should be nullible.</param> | ||
| /// <returns>Type.</returns> | ||
| public override Type GetCSharpType(bool nullible) |
There was a problem hiding this comment.
perhaps you meant nullable instead of nullible?
There was a problem hiding this comment.
I agree with your comment but that name pre-existed in many files in this codebase so I choose to be consistent.
|
I am not sure why this still shows are changes requested, they all have been made to my knowledge. |
|
all changes made |
|
LGTM. Thanks @TomBruns! |
| { TimestampMillisecond.LogicalTypeName, new TimestampMillisecond() }, | ||
| { TimestampMicrosecond.LogicalTypeName, new TimestampMicrosecond() }, | ||
| { Uuid.LogicalTypeName, new Uuid() } | ||
| { Uuid.LogicalTypeName, new Uuid() }, |
|
+1 to this change @zcsizmadia is there anything blocking this from being accepted? |
| case @"double": | ||
| return nullible ? typeof(System.Double?) : typeof(System.Double); | ||
| case @"bytes": | ||
| return nullible ? typeof(System.Byte?[]) : typeof(System.Byte[]); |
There was a problem hiding this comment.
System.Byte?[] is an array type in which each element is a nullable byte. It is not the the correct type for Avro {"type": [ "null", "bytes" ]}, which allows either a null value or an array of non-null bytes. So this should return typeof(System.Byte[]) regardless of nullible, similarly to the case "string" above.
| public override object ConvertToBaseValue(object logicalValue, LogicalSchema schema) | ||
| { | ||
| switch (schema.Name) | ||
| { | ||
| case @"string": | ||
| return (System.String)logicalValue; | ||
| case @"boolean": | ||
| return (System.Boolean)logicalValue; | ||
| case @"int": | ||
| return (System.Int32)logicalValue; | ||
| case @"long": | ||
| return (System.Int64)logicalValue; | ||
| case @"float": | ||
| return (System.Single)logicalValue; | ||
| case @"double": | ||
| return (System.Double)logicalValue; | ||
| case @"bytes": | ||
| return (System.Byte[])logicalValue; | ||
| default: | ||
| return logicalValue; | ||
| } | ||
| } |
There was a problem hiding this comment.
These casts are almost no-ops, because the result of each cast is immediately converted back to object. I think this method could just return logicalValue; always regardless of schema.Name.
The casts have two effects, but I don't think those effects matter here, because the logical value should be the same as the base value, which should be an instance of one of the Avro standard types:
- If the
logicalValueis not compatible with the expected type, then it throws InvalidCastException. - In some cases, the .NET runtime allows unboxing from a type that does not match exactly. For example, casting from Object to Int32 works even if the Object is actually a boxed UInt32 or a boxed enum whose underlying type is Int32 or UInt32.
7 participants
AVRO-2825
What is the purpose of the change
NOTE: This PR WILL change the behavior of the current nuget package. It corrects it to align with the AVRO spec.
This is a new version of PR #2741 incorporating all feedback from KalleOlaviNiemitalo
Avoid swallowing any exceptions thrown for other reasons.
Allow applications to parse a schema and read the name of the logical type from LogicalSchema.LogicalTypeName (or even LogicalType.Name) regardless of whether the library recognizes it.
Allow applications to parse a schema and serialize it back to JSON without losing the unrecognized logical types here:
LogicalSchema.LogicalType could then be null, or perhaps an instance of a new NotSupportedLogicalType class that would pass everything through without conversion. (I created a new type: UnknownLogicalType)
Verifying this change
This change is already covered by existing tests:
test > AvroGen > AvroGenSchemaTests.cs > NotSupportedSchema
test > Schema > SchemaTests.cs > TestUnknownLogical
This change added tests and can be verified as follows:
Documentation