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/01/02 14:20:48 UTC
[GitHub] [incubator-iceberg] Fokko opened a new pull request #722: Remove
warnings from the build
Fokko opened a new pull request #722: Remove warnings from the build
URL: https://github.com/apache/incubator-iceberg/pull/722
Such as:
- Returning both mutable and immutable collections
- Returning mutable collections
- Removing fallthrough in switch statement
- Ungrouped branches in switch statement
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] Fokko commented on a change in pull request
#722: Remove warnings from the build
Posted by GitBox <gi...@apache.org>.
Fokko commented on a change in pull request #722: Remove warnings from the build
URL: https://github.com/apache/incubator-iceberg/pull/722#discussion_r366942238
##########
File path: api/src/main/java/org/apache/iceberg/types/CheckCompatibility.java
##########
@@ -93,7 +93,7 @@ private CheckCompatibility(Schema schema, boolean checkOrdering, boolean checkNu
}
@Override
- public List<String> struct(Types.StructType readStruct, Iterable<List<String>> fieldErrorLists) {
+ public ImmutableList<String> struct(Types.StructType readStruct, Iterable<List<String>> fieldErrorLists) {
Review comment:
Then we should disable this warning:
```
/Users/fokkodriesprong/Desktop/incubator-iceberg/api/src/main/java/org/apache/iceberg/types/CheckCompatibility.java:96: warning: [MixedMutabilityReturnType] This method returns both mutable and immutable collections or maps from different paths. This may be confusing for users of the method.
public List<String> struct(Types.StructType readStruct, Iterable<List<String>> fieldErrorLists) {
```
Personally I prefer to change the signature to `ImmutableList`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] Fokko commented on a change in pull request
#722: Remove warnings from the build
Posted by GitBox <gi...@apache.org>.
Fokko commented on a change in pull request #722: Remove warnings from the build
URL: https://github.com/apache/incubator-iceberg/pull/722#discussion_r366941614
##########
File path: core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java
##########
@@ -53,7 +53,7 @@
private static final String REPLACED_MANIFESTS_COUNT = "manifests-replaced";
private static final String PROCESSED_ENTRY_COUNT = "entries-processed";
- private static final Set<ManifestEntry.Status> ALLOWED_ENTRY_STATUSES = ImmutableSet.of(
+ private static final ImmutableSet<ManifestEntry.Status> ALLOWED_ENTRY_STATUSES = ImmutableSet.of(
Review comment:
```
/Users/fokkodriesprong/Desktop/incubator-iceberg/core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java:56: warning: [MutableConstantField] Constant field declarations should use the immutable type (such as ImmutableList) instead of the general collection interface type (such as List)
private static final Set<ManifestEntry.Status> ALLOWED_ENTRY_STATUSES = ImmutableSet.of(
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#722: Remove warnings from the build
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #722: Remove warnings from the build
URL: https://github.com/apache/incubator-iceberg/pull/722#discussion_r366660235
##########
File path: parquet/src/main/java/org/apache/iceberg/parquet/ParquetFilters.java
##########
@@ -136,29 +136,22 @@ protected Expression bind(UnboundPredicate<?> pred) {
case NOT_EQ:
return FilterApi.notEq(col, getParquetPrimitive(lit));
}
-
+ break;
Review comment:
These changes look good to me.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] Fokko commented on issue #722: Remove warnings
from the build
Posted by GitBox <gi...@apache.org>.
Fokko commented on issue #722: Remove warnings from the build
URL: https://github.com/apache/incubator-iceberg/pull/722#issuecomment-574118381
cc @chenjunjiedada Any thoughts on this?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#722: Remove warnings from the build
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #722: Remove warnings from the build
URL: https://github.com/apache/incubator-iceberg/pull/722#discussion_r367002189
##########
File path: api/src/main/java/org/apache/iceberg/types/CheckCompatibility.java
##########
@@ -93,7 +93,7 @@ private CheckCompatibility(Schema schema, boolean checkOrdering, boolean checkNu
}
@Override
- public List<String> struct(Types.StructType readStruct, Iterable<List<String>> fieldErrorLists) {
+ public ImmutableList<String> struct(Types.StructType readStruct, Iterable<List<String>> fieldErrorLists) {
Review comment:
I think what that message is saying is that sometimes the method returns a mutable list and sometimes it returns an immutable one. I don't think the signature change is required to solve the problem. We just need to make sure that all paths return either immutable or mutable.
We can't change the signature because we don't want to leak Guava in the Iceberg API.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#722: Remove warnings from the build
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #722: Remove warnings from the build
URL: https://github.com/apache/incubator-iceberg/pull/722#discussion_r366660131
##########
File path: core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java
##########
@@ -53,7 +53,7 @@
private static final String REPLACED_MANIFESTS_COUNT = "manifests-replaced";
private static final String PROCESSED_ENTRY_COUNT = "entries-processed";
- private static final Set<ManifestEntry.Status> ALLOWED_ENTRY_STATUSES = ImmutableSet.of(
+ private static final ImmutableSet<ManifestEntry.Status> ALLOWED_ENTRY_STATUSES = ImmutableSet.of(
Review comment:
Why does this need to change?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#722: Remove warnings from the build
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #722: Remove warnings from the build
URL: https://github.com/apache/incubator-iceberg/pull/722#discussion_r366660091
##########
File path: api/src/main/java/org/apache/iceberg/types/CheckCompatibility.java
##########
@@ -93,7 +93,7 @@ private CheckCompatibility(Schema schema, boolean checkOrdering, boolean checkNu
}
@Override
- public List<String> struct(Types.StructType readStruct, Iterable<List<String>> fieldErrorLists) {
+ public ImmutableList<String> struct(Types.StructType readStruct, Iterable<List<String>> fieldErrorLists) {
Review comment:
We don't typically use `ImmutableList` to avoid leaking it in methods that are accidentally public or purposely part of the API. I'm +1 for returning `ImmutableList.copyOf(errors)` below, but I don't think we should guarantee the list will be an `ImmutableList`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue merged pull request #722: Remove
warnings from the build
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #722: Remove warnings from the build
URL: https://github.com/apache/incubator-iceberg/pull/722
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on issue #722: Remove warnings
from the build
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #722: Remove warnings from the build
URL: https://github.com/apache/incubator-iceberg/pull/722#issuecomment-574762817
Thanks for fixing this, @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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org