Skip to content
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

AVRO-2825: [csharp] Resolve: C# Logical Types throw exception on unkn… #2751

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

TomBruns
Copy link

AVRO-2825

What is the purpose of the change

  • This pull request resolves an outstanding issue with the csharp implementation behavior that is not consistent with the AVRO spec and the java behavior.
  • Per the AVRO Spec:
    • Language implementations must ignore unknown logical types when reading, and should use the underlying Avro type
  • The current csharp implementation throws an exception for unrecognized Logical Types.

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

    • corrected expected result of Test
  • test > Schema > SchemaTests.cs > TestUnknownLogical

    • corrected expected result of Test

This change added tests and can be verified as follows:

  • test > Util > LogicalTypeTests.cs > TestUnknownLogicalType
    • added more complex test to confirm underlying AVRO base type is used.

Documentation

  • Does this pull request introduce a new feature? (no)

@github-actions github-actions bot added the C# label Feb 19, 2024
@TomBruns
Copy link
Author

resolved all identified issues in new PR

@TomBruns TomBruns requested a review from martin-g February 21, 2024 14:10
@TomBruns TomBruns requested a review from martin-g February 22, 2024 13:24
@martin-g
Copy link
Member

We need a C# developer/user to review the changes.

@TomBruns
Copy link
Author

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)
Copy link

@a-kalashnikov a-kalashnikov Apr 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps you meant nullable instead of nullible?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your comment but that name pre-existed in many files in this codebase so I choose to be consistent.

@TomBruns
Copy link
Author

TomBruns commented Apr 8, 2024

I am not sure why this still shows are changes requested, they all have been made to my knowledge.

@TomBruns
Copy link
Author

TomBruns commented Apr 8, 2024

all changes made

@TomBruns TomBruns closed this Apr 8, 2024
@TomBruns TomBruns reopened this Apr 8, 2024
@martin-g martin-g dismissed their stale review April 9, 2024 12:35

A C# dev should take a look

@martin-g martin-g requested a review from zcsizmadia April 9, 2024 12:44
@zcsizmadia
Copy link
Contributor

LGTM. Thanks @TomBruns!

@@ -45,7 +45,7 @@ private LogicalTypeFactory()
{ TimeMicrosecond.LogicalTypeName, new TimeMicrosecond() },
{ TimestampMillisecond.LogicalTypeName, new TimestampMillisecond() },
{ TimestampMicrosecond.LogicalTypeName, new TimestampMicrosecond() },
{ Uuid.LogicalTypeName, new Uuid() }
{ Uuid.LogicalTypeName, new Uuid() },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comma plz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants