Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 )
Change subject: IMPALA-7807: Analysis test fixture ...................................................................... Patch Set 3: (14 comments) Some high-level comments about the class structure and a few nits. The refactoring overall makes sense to me. Will take another deeper pass once we finalize on the class structure. I think there is too much logic stuffed into AnalysisFixture class (which also makes it difficult to review). Please refer to the comments about breaking it and if that makes sense. http://gerrit.cloudera.org:8080/#/c/11881/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11881/3//COMMIT_MSG@19 PS3, Line 19: the FrontEndBase was left unchanged. Can you please add a TODO there? There is a lot of common code and this might confuse other developers as to what to use. Also, it is worth mentioning that we suggest using these test fixtures starting now rather than using the "functional" approach. http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java File fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java: http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@55 PS3, Line 55: * Add an example usage here? (something like in ExprRewriterTest) http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@56 PS3, Line 56: The QueryFixture holds per-query state which can include a test-specific : * set of options. The query can be analyzed in full, or only analyzed up : * to rewrites. It provides access to normally-hidden temporary objects : * such as the analyzer, analysis context and so on. Tests can probe : * these objects to check for conditions of interest. : * : * The Rewrite fixture builds on the QueryFixture to add additional : * tools needed to probe rewrites of the various clauses within a : * query. Move to their respective class definitions? http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@80 PS3, Line 80: private final TQueryCtx queryCtx; We generally use "_" suffix for class member variables. For ex: queryCtx_. http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@86 PS3, Line 86: private Analyzer analyzer; Do we need to track this separately? can be inferred from the analysisResult using getAnalyzer() http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@103 PS3, Line 103: e.printStackTrace(); use log4j logger http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@109 PS3, Line 109: We probably also need helper functions like parsesOk() etc? (to port ParserTest) http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@114 PS3, Line 114: makeAnalysisContext move this to ctor? http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@135 PS3, Line 135: public QueryFixture analyzeWithoutRewrite() { I think this also skips authz. Mention that? http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@204 PS3, Line 204: private static class CountingRewriteRuleWrapper implements ExprRewriteRule { Move it inside RewriteFixture? Doesn't seem like to be used else where. http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@205 PS3, Line 205: private int rewrites; Use "_" suffix. http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@224 PS3, Line 224: public class RewriteFixture { Move it to its own class? IIUC AnalysisFixture is a more generic thing that we might use across tests, not sure if it makes sense to put RewriteFixture here. http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@256 PS3, Line 256: query = AnalysisFixture.this.query(stmtStr); Does it make sense for RewriteFixture to extend AnalysisFixture to avoid this boilerplate? http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@284 PS3, Line 284: return query.selectStmt().getSelectList().getItems().get(0).getExpr(); assert that it is analyzed before calling this? -- 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: 3 Gerrit-Owner: Paul Rogers <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 15 Nov 2018 07:05:04 +0000 Gerrit-HasComments: Yes
