Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11881 )
Change subject: IMPALA-7807: Analysis test fixture ...................................................................... Patch Set 4: (14 comments) Thanks for the review! Suggestions applied. Also added a few explanations of future intent. 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 base class was > Can you please add a TODO there? There is a lot of common code and this mig Good suggestions. Added the TODO. My thought is to refine this fixture in another commit, then create an example test that shows folks exactly how to use the fixture, which seems more useful than a general suggestion. Is this OK? 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: this.db_ = db; > Add an example usage here? (something like in ExprRewriterTest) Added note here. Will add a more complete example as this class evolves to absorb more of the existing test framework functionality. http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@56 PS3, Line 56: : : public void setOptions(TQueryOptions queryOptions) { : this.queryOptions_ = queryOptions; : } : : public TQueryOptions getOptions() { : if (queryOptions_ == null) { : que > Move to their respective class definitions? Done http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@80 PS3, Line 80: public TQueryCtx queryContext() { > We generally use "_" suffix for class member variables. For ex: queryCtx_. Done http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@86 PS3, Line 86: * using the options defined > Do we need to track this separately? can be inferred from the analysisResu Done http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@103 PS3, Line 103: return new RewriteFixture(this); > use log4j logger In a test? This is an unexpected test failure. Do we normally want to paw though logs to find such failures? 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 Parser Agreed. I'll add those as I modify code so that I can use those changed tests to test the implementation. http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@114 PS3, Line 114: > move this to ctor? At present this class does analysis in two ways: with or without rewrites. As I learn this stuff better, I suspect it would be better to split this into two classes. I'll save that for the next commit, if you agree. http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@135 PS3, Line 135: > I think this also skips authz. Mention that? Done http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@204 PS3, Line 204: > Move it inside RewriteFixture? Doesn't seem like to be used else where. Done http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@205 PS3, Line 205: > Use "_" suffix. Done http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@224 PS3, Line 224: > Move it to its own class? IIUC AnalysisFixture is a more generic thing tha Done http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@256 PS3, Line 256: > Does it make sense for RewriteFixture to extend AnalysisFixture to avoid th Rewriting has some awkwardness that requires that things be in a different order here than in the base class. So, reuse by composition works a bit better here than reuse via inheritance. http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@284 PS3, Line 284: > assert that it is analyzed before calling this? Good idea in general. Here, the structure exists after parse, analysis only "decorates" this structure. -- 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: 4 Gerrit-Owner: Paul Rogers <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Comment-Date: Thu, 15 Nov 2018 22:18:40 +0000 Gerrit-HasComments: Yes
