Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12221 )
Change subject: IMPALA-5872: Testcase builder for query planner ...................................................................... Patch Set 3: (13 comments) 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 Makes sense 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 Initially, I did something like this (in PS1) and Greg suggested COPY..grammar. I'm not too strong about either. Copy here is probably in the sense that we are copying some form of tables to a file in the output dir or something. Lemme know if you feel strongly about it, I'll do the refactor. It is a bit tedious. 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 po Redid the whole thing. 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); > i would still make it final and just set it to null in the Catalog() constr Done http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java@97 PS3, Line 97: , > nit: space after comma Done 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 indica Done http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@289 PS3, Line 289: > nit: remove extra line Done 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. C Added a TODO here. This would be an involved change and the current patch is already too big. We can probably defer this to later change? http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1176 PS3, Line 1176: // Exit early if all hosts have a scan range assignment, to avoid extraneous work > line too long (91 > 90) Done 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? Interestingly we have it reserved here [1]. I figured that after making the changes. It threw an error saying that reserved word cannot be used as an identifier. [1] https://github.com/apache/impala/blob/master/fe/src/main/jflex/sql-scanner.flex#L326 http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@368 PS3, Line 368: > nit: fix indentation Done http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/test/java/org/apache/impala/testutil/EmbeddedMetastoreClientPool.java File fe/src/test/java/org/apache/impala/testutil/EmbeddedMetastoreClientPool.java: http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/test/java/org/apache/impala/testutil/EmbeddedMetastoreClientPool.java@35 PS3, Line 35: > nit: extra space Done http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/test/java/org/apache/impala/testutil/EmbeddedMetastoreClientPool.java@42 PS3, Line 42: private Path derbyDataStorePath_; > can be final Done -- 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: Fri, 01 Feb 2019 02:16:47 +0000 Gerrit-HasComments: Yes
