Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11893 )
Change subject: IMPALA-7823: Clean up Java warnings, fix minor issues ...................................................................... Patch Set 5: (28 comments) My general comments: - We should aim to fix the warnings instead of suppressing them whenever possible. - Possible enable -Werror to fail on warnings in the future. - There are many possible warnings (and logic bugs) by running IntelliJ code analyzer that are not covered by this CR and I don't think it should be covered by this CR either. It's a good idea to break this CR into multiple pieces to make it easier to review. - Inconsistent usage of log4j and slf4j. We should decide whether we want to use log4j or slf4j in FE, but not both. http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@1935 PS5, Line 1935: <SlotId, SlotId> Java 8 type inference is a lot smarter, it can infer the type. Thus, this can be simplified to new Pair<>(id1, id2) http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java File fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java@37 PS5, Line 37: nit: remove extra new line. Our convention (although not universal) is to not have a new line after a class declaration. http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java File fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@112 PS5, Line 112: nit: remove extra new line. http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/catalog/CatalogException.java File fe/src/main/java/org/apache/impala/catalog/CatalogException.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/catalog/CatalogException.java@27 PS5, Line 27: nit: remove extra line http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@663 PS5, Line 663: Boolean Use Reference<>(false) instead. http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java File fe/src/main/java/org/apache/impala/planner/JoinNode.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java@551 PS5, Line 551: @SuppressWarnings("incomplete-switch") Should we use default: break instead? This way the intention is clear and there is no need to suppress the warning. http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@411 PS5, Line 411: new Pair<TableRef, Long> use new Pair<> form instead http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@420 PS5, Line 420: new Pair<TableRef, Long> use new Pair<> form instead http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java File fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java@37 PS5, Line 37: public static ExprRewriteRule INSTANCE = new BetweenToCompoundRule(); this should be final http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java@38 PS5, Line 38: also add private constructor to prevent instantiation. http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java@50 PS5, Line 50: } else { : // Rewrite into conjunction. : Predicate lower = new BinaryPredicate(BinaryPredicate.Operator.GE, : bp.getChild(0), bp.getChild(1)); : Predicate upper = new BinaryPredicate(BinaryPredicate.Operator.LE, : bp.getChild(0), bp.getChild(2)); : return new CompoundPredicate(CompoundPredicate.Operator.AND, lower, upper); : } I don't know what's the convention in Impala, but I know in a different programming language the preferred style is to not have else it's redundant. http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java File fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java@39 PS5, Line 39: nit: remove extra new line http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java@40 PS5, Line 40: public static ExprRewriteRule INSTANCE = new EqualityDisjunctsToInRule(); this should be final http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java@42 PS5, Line 42: nit: remove space http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java File fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java@37 PS5, Line 37: public static ExprRewriteRule INSTANCE = new NormalizeBinaryPredicatesRule(); this should be final http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java@39 PS5, Line 39: nit: remove space http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java File fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java@33 PS5, Line 33: public static ExprRewriteRule INSTANCE = new NormalizeExprsRule(); this should be final http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java@35 PS5, Line 35: nit: remove extra space http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java File fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@60 PS5, Line 60: remove extra space http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@56 PS5, Line 56: public static ExprRewriteRule INSTANCE = new SimplifyConditionalsRule(); this should be final http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@61 PS5, Line 61: nit: remove extra space http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java File fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java@35 PS5, Line 35: public static ExprRewriteRule INSTANCE = new SimplifyDistinctFromRule(); this should be final http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java@37 PS5, Line 37: remove extra space http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3714 PS5, Line 3714: throw new CatalogException("Database: " + dbName + " does not exist."); nice catch http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/service/JniFrontend.java@727 PS5, Line 727: Enumeration I think we can replace it with Enumeration<?> instead of using @SuppressWarning http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/main/java/org/apache/impala/service/JniFrontend.java@805 PS5, Line 805: fs instanceof SecureAzureBlobFileSystem I believe this condition is useless since we do a check in fs instanceof AzureBlobFileSystem and SecureAzureBlobFileSystem extends from AzureBlobFileSystem. http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@406 PS5, Line 406: @SuppressWarnings("unchecked") Shouldn't this be @SuppressWarnings("varargs") instead? http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java: http://gerrit.cloudera.org:8080/#/c/11893/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@131 PS5, Line 131: Collection fix this to Collection<TestContext> instead of suppressing it. -- To view, visit http://gerrit.cloudera.org:8080/11893 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b174d6448371c55e98487c6d4212c74c6b0c79e Gerrit-Change-Number: 11893 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers <par0...@yahoo.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <par0...@yahoo.com> Gerrit-Comment-Date: Tue, 20 Nov 2018 20:28:37 +0000 Gerrit-HasComments: Yes
