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 2020/12/30 15:46:52 UTC
[GitHub] [iceberg] Fokko opened a new pull request #2012: Fix small issues
Fokko opened a new pull request #2012:
URL: https://github.com/apache/iceberg/pull/2012
While working on https://github.com/apache/iceberg/pull/1972 I've noticed that there are a lot of warnings in the output. This PR fixes the low hanging fruit to reduce the noise.
----------------------------------------------------------------
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.
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] fbocse commented on a change in pull request #2012: Fix small issues
Posted by GitBox <gi...@apache.org>.
fbocse commented on a change in pull request #2012:
URL: https://github.com/apache/iceberg/pull/2012#discussion_r551342792
##########
File path: core/src/main/java/org/apache/iceberg/ScanSummary.java
##########
@@ -305,7 +305,7 @@ public String toString() {
private static class TopN<K, V> {
private final int maxSize;
private final boolean throwIfLimited;
- private final SortedMap<K, V> map;
+ private final NavigableMap<K, V> map;
Review comment:
Curious to what the issue that this change fixes? I'm asking cause I'm not really that knowledgeable when it comes to these java collections in particular, all I know is that SortedMap is a superinterface of NavigableMap, and since I'm generally guided by using the least specific interface "principle" I was wondering why that wouldn't apply in this case as well.
----------------------------------------------------------------
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.
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] fbocse commented on a change in pull request #2012: Fix small issues
Posted by GitBox <gi...@apache.org>.
fbocse commented on a change in pull request #2012:
URL: https://github.com/apache/iceberg/pull/2012#discussion_r551342792
##########
File path: core/src/main/java/org/apache/iceberg/ScanSummary.java
##########
@@ -305,7 +305,7 @@ public String toString() {
private static class TopN<K, V> {
private final int maxSize;
private final boolean throwIfLimited;
- private final SortedMap<K, V> map;
+ private final NavigableMap<K, V> map;
Review comment:
Curious to what the issue that this change fixes? I'm asking cause I'm not really that knowledgeable when it comes to these java collections in particular, all I know is that SortedMap is a superinterface of NavigableMap basically, and since I'm generally guided by using the least specific interface "principle" I was wondering why that wouldn't apply in this case as well.
----------------------------------------------------------------
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.
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] Fokko commented on a change in pull request #2012: Fix small issues
Posted by GitBox <gi...@apache.org>.
Fokko commented on a change in pull request #2012:
URL: https://github.com/apache/iceberg/pull/2012#discussion_r551582770
##########
File path: core/src/main/java/org/apache/iceberg/ScanSummary.java
##########
@@ -305,7 +305,7 @@ public String toString() {
private static class TopN<K, V> {
private final int maxSize;
private final boolean throwIfLimited;
- private final SortedMap<K, V> map;
+ private final NavigableMap<K, V> map;
Review comment:
Great question. This one triggered me:
```
/Users/fokkodriesprong/Desktop/incubator-iceberg/core/src/main/java/org/apache/iceberg/ScanSummary.java:326: warning: [JdkObsolete] SortedMap was replaced by NavigableMap in Java 6.
map.put(key, updateFunc.apply(map.get(key)));
^
(see https://errorprone.info/bugpattern/JdkObsolete)
```
The SortedMap has been superseded by the NavigableMap. According to the author, there are some issues with the sorted set API: https://www.youtube.com/watch?v=aAb7hSCtvGw#t=1258 I agree that in this case, it doesn't really matter.
----------------------------------------------------------------
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.
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] rdblue commented on pull request #2012: Fix small issues
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2012:
URL: https://github.com/apache/iceberg/pull/2012#issuecomment-754280709
Thanks, @Fokko!
----------------------------------------------------------------
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.
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] rdblue merged pull request #2012: Fix small issues
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #2012:
URL: https://github.com/apache/iceberg/pull/2012
----------------------------------------------------------------
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.
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