You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2021/08/13 00:59:51 UTC
[GitHub] [avro] yxmm-wxe opened a new pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
yxmm-wxe opened a new pull request #1303:
URL: https://github.com/apache/avro/pull/1303
Make sure you have checked _all_ steps below.
### Jira
- [ ] My PR addresses the following [Avro Jira](https://issues.apache.org/jira/browse/AVRO/) issues and references them in the PR title. For example, "AVRO-1234: My Avro PR"
- https://issues.apache.org/jira/browse/AVRO-3182
- In case you are adding a dependency, check if the license complies with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
### Commits
- [ ] My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](https://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [ ] In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on pull request #1303:
URL: https://github.com/apache/avro/pull/1303#issuecomment-904627082
I'm not too worried about _existing_ CodeQL scanning warnings, although i appreciate that you took a bit of extra time to fix them. When you are happy with your solution, let me know!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] yxmm-wxe edited a comment on pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
yxmm-wxe edited a comment on pull request #1303:
URL: https://github.com/apache/avro/pull/1303#issuecomment-903386309
Hi, thank you for your comment.
1) I have added the unit test that demonstrates the fix.
2) I'm not clear why UnionSchema was included here in the first place, but I found Shema.ParseJson is compatible with both cases: [Parse union schema json without type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L174) and [Parse union schema json with type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L203).
All unit tests are passed, and with or without type property, Schema.ParseJson is compatible in the source code.
Furthermore, I used Avro in C# to parse the Avro stream from Java, it works correctly in the real project.
> UnionSchema.ToString() in Java also does not contain type property.
Thus, I think it has no impact on a field with a UNION.
3) About the Code scanning warnings.
Firstly, the Schema is an abstract class, so I don't think it will introduce a dependency cycle. And this code is written like this at the beginning.
Secondly, it is strange to me to judge the derived class in the parent class.
So I override ToString() in PrimitiveSchema separately. And all unit tests are passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] yxmm-wxe edited a comment on pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
yxmm-wxe edited a comment on pull request #1303:
URL: https://github.com/apache/avro/pull/1303#issuecomment-903386309
Hi, thank you for your comment.
1) I have added the unit test that demonstrates the fix.
2) I'm not clear why UnionSchema was included here in the first place, but I found Shema.ParseJson is compatible with both cases: [Parse union schema json without type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L174) and [Parse union schema json with type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L203).
All unit tests are passed, and with or without type property, Schema.ParseJson is compatible in the source code.
Furthermore, I used Avro in C# to parse the Avro stream from Java, it works correctly in the real project.
> UnionSchema.ToString() in Java also does not contain type property.
Thus, I think it has no impact on a field with a UNION.
3) About the Code scanning warnings.
Firstly, the Schema is an abstract class, so I don't think it will introduce a dependency cycle. And this code is written like this at the beginning.
Secondly, it is strange to me to judge the derived class in the parent class.
So I override ToString() in PrimitiveSchema separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] yxmm-wxe edited a comment on pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
yxmm-wxe edited a comment on pull request #1303:
URL: https://github.com/apache/avro/pull/1303#issuecomment-903386309
Hi, thank you for your comment.
1) I have added the unit test that demonstrates the fix.
2) I'm not clear why UnionSchema was included here in the first place, but I found Shema.ParseJson is compatible with both cases: [Parse union schema json without type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L174) and [Parse union schema json with type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L203).
All unit tests are passed, and with or without type property, Schema.ParseJson is compatible in the source code.
Furthermore, I used Avro in C# to parse the Avro stream from Java, it works correctly in the real project.
> UnionSchema.ToString() in Java also does not contain type property.
Thus, I think it has no impact on a field with a UNION.
Another suggestion: Could we try to remove parsing union schema json with type property after this fix?
3) About the Code scanning warnings.
Firstly, the Schema is an abstract class, so I don't think it will introduce a dependency cycle. And this code is written like this at the beginning.
Secondly, it is strange to me to judge the derived class in the parent class.
So I override ToString() in PrimitiveSchema separately. And all unit tests are passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] yxmm-wxe edited a comment on pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
yxmm-wxe edited a comment on pull request #1303:
URL: https://github.com/apache/avro/pull/1303#issuecomment-903386309
Hi, thank you for your comment.
1) I have added the unit test that demonstrates the fix.
2) I'm not clear why UnionSchema was included here in the first place, but I found Shema.ParseJson is compatible with both cases: [Parse union schema json without type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L174) and [Parse union schema json with type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L203).
All unit tests are passed, and with or without type property, Schema.ParseJson is compatible in the source code.
Furthermore, I used Avro in C# to parse the Avro stream from Java, it works correctly in the real project.
> UnionSchema.ToString() in Java also does not contain type property.
Thus, I think it has no impact on a field with a UNION.
Another suggestion: Could we try to remove parsing union schema json with type property after this fix.
3) About the Code scanning warnings.
Firstly, the Schema is an abstract class, so I don't think it will introduce a dependency cycle. And this code is written like this at the beginning.
Secondly, it is strange to me to judge the derived class in the parent class.
So I override ToString() in PrimitiveSchema separately. And all unit tests are passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] yxmm-wxe commented on pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
yxmm-wxe commented on pull request #1303:
URL: https://github.com/apache/avro/pull/1303#issuecomment-904465480
I am troubled with the previous code implementation. My changes trigger the code scan warning again. I guess there is no code scanning at the first commit.
@RyanSkraba Please help approve running workflows again. Thank you very much!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] yxmm-wxe edited a comment on pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
yxmm-wxe edited a comment on pull request #1303:
URL: https://github.com/apache/avro/pull/1303#issuecomment-903386309
Hi, thank you for your comment.
1) I have added the unit test that demonstrates the fix.
2) I'm not clear why UnionSchema was included here in the first place, but I found Shema.ParseJson is compatible with both cases: [Parse union schema json without type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L174) and [Parse union schema json with type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L203)
3) About the Code scanning warnings.
Firstly, the Schema is an abstract class, so I don't think it will introduce a dependency cycle. And this code is written like this at the beginning.
Secondly, it is strange to me to judge the derived class in the parent class.
So I override ToString() in PrimitiveSchema separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] yxmm-wxe edited a comment on pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
yxmm-wxe edited a comment on pull request #1303:
URL: https://github.com/apache/avro/pull/1303#issuecomment-903386309
Hi, thank you for your comment.
1) I have added the unit test that demonstrates the fix.
2) I'm not clear why UnionSchema was included here in the first place. The code was implemented 10 years ago. But I found Shema.ParseJson is compatible with both cases: [Parse union schema json without type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L174) and [Parse union schema json with type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L203).
All unit tests are passed, and with or without type property, Schema.ParseJson is compatible in the source code.
Furthermore, I used Avro in C# to parse the Avro stream from Java, it works correctly in the real project.
> UnionSchema.ToString() in Java also does not contain type property.
Thus, I think it has no impact on a field with a UNION.
Another suggestion: Could we try to remove parsing union schema json with type property after this fix?
3) About the Code scanning warnings.
Firstly, the Schema is an abstract class, so I don't think it will introduce a dependency cycle. And this code is written like this at the beginning.
Secondly, it is strange to me to judge the derived class in the parent class.
So I override ToString() in PrimitiveSchema separately. And all unit tests are passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] yxmm-wxe edited a comment on pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
yxmm-wxe edited a comment on pull request #1303:
URL: https://github.com/apache/avro/pull/1303#issuecomment-903386309
Hi, thank you for your comment.
1) I have added the unit test that demonstrates the fix.
2) I'm not clear why UnionSchema was included here in the first place, but I found Shema.ParseJson is compatible with both cases: [Parse union schema json without type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L174) and [Parse union schema json with type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L203).
All unit tests are passed, and with or without type property, Schema.ParseJson is compatible.
Furthermore, I use Avro in C# to parse the Avro stream from Java, it works correctly in the real project.
Thus, I think it has no impact on a field with a UNION.
3) About the Code scanning warnings.
Firstly, the Schema is an abstract class, so I don't think it will introduce a dependency cycle. And this code is written like this at the beginning.
Secondly, it is strange to me to judge the derived class in the parent class.
So I override ToString() in PrimitiveSchema separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] yxmm-wxe commented on pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
yxmm-wxe commented on pull request #1303:
URL: https://github.com/apache/avro/pull/1303#issuecomment-903386309
Hi, thank you for your comment.
1) I have added the unit test that demonstrates the fix.
2) I'm not clear why UnionSchema was included here in the first place, but I found Shema.ParseJson is compatible with both cases: [Parse union schema json without type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L174) and [Parse union schema json with type property](https://github.com/apache/avro/blob/master/lang/csharp/src/apache/main/Schema/Schema.cs#L203)
3) About the Code scanning warnings.
Firstly, the Schema is an abstract class, so I don't think it will introduce a dependency circle. And this code is written like this at the beginning.
Secondly, it is strange to me to judge the derived class in the parent class.
So I override ToString() in PrimitiveSchema separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on pull request #1303:
URL: https://github.com/apache/avro/pull/1303#issuecomment-904445082
Thanks so much -- I wasn't sure what the code scanning warnings meant, so thanks for going deeper into those.
This LGTM, let's see if any C# experts weigh in over the next few days!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] yxmm-wxe edited a comment on pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
yxmm-wxe edited a comment on pull request #1303:
URL: https://github.com/apache/avro/pull/1303#issuecomment-904465480
I am troubled with the initial code implementation. My changes trigger the code scanning warning again. I guess there is no code scanning at the first commit.
@RyanSkraba Please help approve running workflows again. Thank you very much!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba commented on pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
RyanSkraba commented on pull request #1303:
URL: https://github.com/apache/avro/pull/1303#issuecomment-913648616
Thanks for the contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] yxmm-wxe edited a comment on pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
yxmm-wxe edited a comment on pull request #1303:
URL: https://github.com/apache/avro/pull/1303#issuecomment-904465480
I am troubled with the initial code implementation. My changes trigger the code scan warning again. I guess there is no code scanning at the first commit.
@RyanSkraba Please help approve running workflows again. Thank you very much!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] RyanSkraba merged pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
RyanSkraba merged pull request #1303:
URL: https://github.com/apache/avro/pull/1303
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] yxmm-wxe commented on pull request #1303: AVRO-3182 fix union schema ToString() has an extra type property
Posted by GitBox <gi...@apache.org>.
yxmm-wxe commented on pull request #1303:
URL: https://github.com/apache/avro/pull/1303#issuecomment-904660229
@RyanSkraba I have fixed all CodeQL scanning warnings in my changes. Maybe I have code cleanliness. (^_^) Thanks for your appreciation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org