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

WIP: AVRO-3986: [csharp] - Plain JSON encoding for Apache Avro #2888

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

clemensv
Copy link
Contributor

@clemensv clemensv commented May 2, 2024

[Do not merge.]

This PR accompanies my filing of AVRO-3986 and implements all features defined in the issue. Purpose of the PR is also to illustrate the scope of the work for the other languages. New tests have been added to account for the different modes and the added features.

It's obviously possible to break up the umbrella issue into multiple issues and to break up the PR into separate issues, but since the interoperability goal is only achieved by the sum of the features, I put it all into one. The majority of the new code is related to union handling (features 5 and 6) and RFC3339 date-time handling.

@github-actions github-actions bot added the C# label May 2, 2024
@opwvhk opwvhk changed the title AVRO-3986: [csharp] - Plain JSON encoding for Apache Avro WIP: AVRO-3986: [csharp] - Plain JSON encoding for Apache Avro May 2, 2024
@github-actions github-actions bot added Java Pull Requests for Java binding build website Rust labels May 15, 2024
@Xuanwo
Copy link
Member

Xuanwo commented Jul 31, 2024

Hi, I believe this PR has been marked as rust in wrong.

@martin-g
Copy link
Member

martin-g commented Aug 1, 2024

Hi, I believe this PR has been marked as rust in wrong.

The PR diff shows modified files in C#, Java and Rust.
There are 26 commits by several authors (me included) - that makes me think the author needs to rebase to latest main, or create a fresh new branch/PR and cherry-pick only his commits!

@martin-g martin-g marked this pull request as draft August 1, 2024 07:12
@clemensv
Copy link
Contributor Author

Hi, I believe this PR has been marked as rust in wrong.

The PR diff shows modified files in C#, Java and Rust. There are 26 commits by several authors (me included) - that makes me think the author needs to rebase to latest main, or create a fresh new branch/PR and cherry-pick only his commits!

I bungled a merge during an update and dragged a bunch of stuff in that didn't belong. I just rebased and cleaned that up.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@clemensv
Copy link
Contributor Author

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Interesting. The tool contains about upcasts of the form

(T)(object)((DateTime)logicalValue).ToUniversalTime();

whereby the upcast to object from DateTime is flagged. The cast is required for the cast to (T) to compile and will indeed fail at runtime if there's a mismatch. So that looks like a false positive.

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.

3 participants