Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12221 )
Change subject: IMPALA-5872: Testcase builder for query planner ...................................................................... Patch Set 2: (19 comments) http://gerrit.cloudera.org:8080/#/c/12221/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12221/2//COMMIT_MSG@41 PS2, Line 41: nit: remove new line http://gerrit.cloudera.org:8080/#/c/12221/2/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/12221/2/common/thrift/Frontend.thrift@915 PS2, Line 915: // Query statemnt for which this test case data is generated. : 1: required string query_stmt : // All referenced table and view defs. : 2: optional list<CatalogObjects.TTable> tables_and_views : // All databases referenced in the query. : 3: optional list<CatalogObjects.TDatabase> dbs : // Output path : 4: required string testcase_data_path : // Impala version that was used to generate this testcase. : // TODO: How to deal with version incompatibilities? E.g: A testcase collected on : // Impala version v1 may or may not be compatible to Impala version v2 if the : // underlying thrift layout changes. : 5: required string impala_version nit: Adding a new line after each field makes it a bit more readable. http://gerrit.cloudera.org:8080/#/c/12221/2/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/12221/2/common/thrift/JniCatalog.thrift@747 PS2, Line 747: nit: remove extra new lines. http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/cup/sql-parser.cup@769 PS2, Line 769: KW_TESTCASE I don't think "testcase" is a reserved SQL keyword. So, instead of introducing a new "testcase" keyword, should we use ident trick? KW_COPY IDENT:ident KW_TO STRING_LITERAL:path query_sttmt:query {: if (!ident.toUppercase().equals("TESTCASE") { parser.parseError(....) } ... :} http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java File fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java: http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@62 PS2, Line 62: testOutputFilePrefix_ nit: use upper case variable name since it's static final http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@82 PS2, Line 82: } nit: add new line http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@107 PS2, Line 107: true, true add inline comments for true, true http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@114 PS2, Line 114: true, true add inline comments for true, true http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@133 PS2, Line 133: Table table = (Table) referencedTable.getTable(); Add Preconditions.checkState(referencedTable.getTable() instanceof Table) http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@151 PS2, Line 151: View view = (View) feView; Add Preconditions.checkState() http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/CopyTestCaseStmt.java@181 PS2, Line 181: os.close(); Wrap in try/finally or use try resource. http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/HdfsUri.java File fe/src/main/java/org/apache/impala/analysis/HdfsUri.java: http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/analysis/HdfsUri.java@55 PS2, Line 55: false Add comment /* throwIfNotExists */ false to easily distinguish between registerPrivReq vs throwIfNotExists. http://gerrit.cloudera.org:8080/#/c/12221/2/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/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1306 PS2, Line 1306: } nit: add new line http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/catalog/FeTable.java File fe/src/main/java/org/apache/impala/catalog/FeTable.java: http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/catalog/FeTable.java@34 PS2, Line 34: nameComparator_ Variables in an interface are essentially static final, so using upper case for the variable name is more appropriate, i.e. NAME_COMPARATOR. We could also use lambda for this: Comparator<FeTable> NAME_COMPARATOR = (t1, t2) -> t1.getFullName().compareTo(t2.getFullName()); http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java: http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@533 PS2, Line 533: return false Returning false when an IOException was thrown doesn't necessarily mean it's not a directory. Maybe we should just re-throw the exception instead? http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@432 PS2, Line 432: nit: remove space http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@437 PS2, Line 437: responseStr wouldn't it be better to specify the test case path in the responseStr? http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/main/java/org/apache/impala/service/JniFrontend.java@20 PS2, Line 20: import java.io.BufferedInputStream; Are these new imports unused? http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java File fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java: http://gerrit.cloudera.org:8080/#/c/12221/2/fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java@80 PS2, Line 80: System.exit(0); nit: this isn't necessary -- 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: 2 Gerrit-Owner: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Balazs Jeszenszky <jes...@gmail.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Comment-Date: Wed, 23 Jan 2019 18:21:11 +0000 Gerrit-HasComments: Yes
