Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 )
Change subject: IMPALA-7807: Analysis test fixture ...................................................................... Patch Set 11: (6 comments) Thanks for the reviews. Addressed comments and rebased on master. http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java File fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java: http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/AnalysisSessionFixture.java@171 PS10, Line 171: public StatementBase statement() { return stmt_; } > We need a Preconditions to check if the statement has been analyzed or else Actually, if you look inside the checkNull assertion, it simply throws an NPE if the object is null. The key value of that check is to clearly state in the code that something is not supposed to be null. In a more complex method, it can check invariants at the top to avoid tracking down bigs inside a complex bit of code. Here, the code is a one-liner: if the analysis result is null, Java will throw an NPE exactly as if the Preconditions.checkNotNull() were called. And, someone looking at the stack should have pretty good idea what is wrong. So, the question then becomes, does the Preconditions check add value beyond what the code itself already checks? I think the answer here is that the message associated with the Preconditions can help a developer catch errors when learning now to use the fixture. So, added Preconditions here and elsewhere with messages to explain correct usage. http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@75 PS10, Line 75: @Override > Similarly, add Preconditions to check if the statement has been analyzed. Done http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@78 PS10, Line 78: turn analyzer_; > nit: we can probably join L78 with L77 Done http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@90 PS10, Line 90: for (ExprRewriteRule r : rules) { > nit: remove new line Done http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@160 PS10, Line 160: > line too long (106 > 90) Done http://gerrit.cloudera.org:8080/#/c/11881/10/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@174 PS10, Line 174: } > line too long (106 > 90) Done -- To view, visit http://gerrit.cloudera.org:8080/11881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c Gerrit-Change-Number: 11881 Gerrit-PatchSet: 11 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: Fri, 14 Dec 2018 20:44:40 +0000 Gerrit-HasComments: Yes
