Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12221 )
Change subject: IMPALA-5872: Testcase builder for query planner ...................................................................... Patch Set 3: (6 comments) Just a few minor comments. As noted before, we'll be able to refine this much quicker once it is in master and we can try it out. http://gerrit.cloudera.org:8080/#/c/12221/3/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/12221/3/be/src/service/query-options.h@44 PS3, Line 44: TImpalaQueryOptions::PLANNER_DEBUG_MODE + 1);\ Now that you've added all the plumbing... I wonder if "debug mode" isn't a bit too generic. Is "debug mode" what we do when we run PlannerTest in Eclipse? Maybe "Testcase mode" to match the test case builder idea? Or "dry run mode" to indicate that we can plan the query, but not run it? http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/cup/sql-parser.cup@770 PS3, Line 770: KW_COPY testcase_ident:testcase KW_TO STRING_LITERAL:path query_stmt:query COPY seems an odd keyword. COPY usually means we have one of something and we make a second copy. Can we use CREATE to make the test case, and LOAD to load the test case into memory? Then, do we need a REMOVE command to get rid of the cached object so a restart isn't needed? http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java File fe/src/main/java/org/apache/impala/catalog/Catalog.java: http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java@66 PS3, Line 66: protected MetaStoreClientPool metaStoreClientPool_ = new MetaStoreClientPool(0, 0); Making this non-final means that the variable can be changed: the client pool can be replaced. Is that the intent? If you only want to change the client pool itself, then the variable can still be final. (Final in Java != const in C++...) http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java@98 PS3, Line 98: metaStoreClientPool) { The client pool is passed in via the constructor. That is usually an indication that the value of the assigned variable can't change during the life of the constructed object. http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1126 PS3, Line 1126: if (analyzer.getQueryOptions().planner_debug_mode) { Suggestion: adding modes and flags like this is brittle and hard to test. Consider that "file system proxy" class that we discussed earlier. http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/jflex/sql-scanner.flex File fe/src/main/jflex/sql-scanner.flex: http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/jflex/sql-scanner.flex@99 PS3, Line 99: keywordMap.put("copy", SqlParserSymbols.KW_COPY); This ads a new keyword. Will this cause existing queries to fail? SELECT copy FROM myTable; Will work prior to this change, fail after. Can we reuse existing keywords? -- To view, visit http://gerrit.cloudera.org:8080/12221 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec83eeb2dc5136768b70ed581fb8d3ed0335cb52 Gerrit-Change-Number: 12221 Gerrit-PatchSet: 3 Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Balazs Jeszenszky <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Comment-Date: Wed, 30 Jan 2019 20:12:03 +0000 Gerrit-HasComments: Yes
