You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/08/15 18:53:10 UTC
[GitHub] [iceberg] JonasJ-ap opened a new pull request, #5541: Build: Resolve one unchecked type cast in TestAvroNameMapping
JonasJ-ap opened a new pull request, #5541:
URL: https://github.com/apache/iceberg/pull/5541
Using recursive type check to ensure every element in the map has desired type when casting
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #5541: Build: Resolve unchecked Map type cast in TestAvroNameMapping
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on PR #5541:
URL: https://github.com/apache/iceberg/pull/5541#issuecomment-1216011248
@kbendick any additional comments?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5541: Build: Resolve unchecked Map type cast in TestAvroNameMapping
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5541:
URL: https://github.com/apache/iceberg/pull/5541#discussion_r946120054
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroNameMapping.java:
##########
@@ -102,6 +102,7 @@ public void testMapProjections() throws IOException {
Types.NestedField.required(1, "lat", Types.FloatType.get()))))));
projected = writeAndRead(writeSchema, readSchema, record, nameMapping);
+ @SuppressWarnings("unchecked")
Review Comment:
oh I see this comes from 2 methods. But even in that case I think it's always good practice to use annotation only in method declaration if possible
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] JonasJ-ap commented on a diff in pull request #5541: Build: Resolve unchecked Map type cast in TestAvroNameMapping
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5541:
URL: https://github.com/apache/iceberg/pull/5541#discussion_r946135997
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroNameMapping.java:
##########
@@ -102,6 +102,7 @@ public void testMapProjections() throws IOException {
Types.NestedField.required(1, "lat", Types.FloatType.get()))))));
projected = writeAndRead(writeSchema, readSchema, record, nameMapping);
+ @SuppressWarnings("unchecked")
Review Comment:
Thank you for your suggestion. Since there are in total 5 unchecked warnings (2 Map types and 3 List types). I think it may be better to place one SuppressWarnings on top of the class to eliminate all the unchecked warning messages.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5541: Build: Resolve unchecked Map type cast in TestAvroNameMapping
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5541:
URL: https://github.com/apache/iceberg/pull/5541#discussion_r946112533
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroNameMapping.java:
##########
@@ -102,6 +102,7 @@ public void testMapProjections() throws IOException {
Types.NestedField.required(1, "lat", Types.FloatType.get()))))));
projected = writeAndRead(writeSchema, readSchema, record, nameMapping);
+ @SuppressWarnings("unchecked")
Review Comment:
I think we can directly place @SuppressWarnings on top of the method, so we only need 1
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroNameMapping.java:
##########
@@ -102,6 +102,7 @@ public void testMapProjections() throws IOException {
Types.NestedField.required(1, "lat", Types.FloatType.get()))))));
projected = writeAndRead(writeSchema, readSchema, record, nameMapping);
+ @SuppressWarnings("unchecked")
Review Comment:
I think we can directly place `@SuppressWarnings` on top of the method, so we only need 1
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] JonasJ-ap commented on pull request #5541: Build: Resolve unchecked Map type cast in TestAvroNameMapping
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on PR #5541:
URL: https://github.com/apache/iceberg/pull/5541#issuecomment-1215773607
Thank you for your suggestions. The recursive type check is now replaced by two SupressWarnings
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 merged pull request #5541: Build: Resolve unchecked Map type cast in TestAvroNameMapping
Posted by GitBox <gi...@apache.org>.
jackye1995 merged PR #5541:
URL: https://github.com/apache/iceberg/pull/5541
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #5541: Build: Resolve one unchecked type cast in TestAvroNameMapping
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on PR #5541:
URL: https://github.com/apache/iceberg/pull/5541#issuecomment-1215672529
thanks for the fix, I don't think we need to recursively cast, and after doing that we still need to do SuppressWarning. Given this is just a test, I think we can just add `SuppressWarning` to the test?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #5541: Build: Resolve one unchecked type cast in TestAvroNameMapping
Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #5541:
URL: https://github.com/apache/iceberg/pull/5541#issuecomment-1215704203
> thanks for the fix, I don't think we need to recursively cast, and after doing that we still need to do SuppressWarning. Given this is just a test, I think we can just add `SuppressWarning` to the test?
Agreed. Since this is just test code, the potentially unsafe cast isn't really a concern here and it would be cleaner to simply add the suppression at the test level.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #5541: Build: Resolve unchecked Map type cast in TestAvroNameMapping
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on code in PR #5541:
URL: https://github.com/apache/iceberg/pull/5541#discussion_r946149225
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroNameMapping.java:
##########
@@ -102,6 +102,7 @@ public void testMapProjections() throws IOException {
Types.NestedField.required(1, "lat", Types.FloatType.get()))))));
projected = writeAndRead(writeSchema, readSchema, record, nameMapping);
+ @SuppressWarnings("unchecked")
Review Comment:
👌
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org