Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14731 )
Change subject: IMPALA-9158: Support loading primary key/foreign key constraints in LocalCatalog Mode. ...................................................................... Patch Set 3: (11 comments) Thanks for your initial set of comments. Addressed them below. http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/main/java/org/apache/impala/analysis/TableDef.java File fe/src/main/java/org/apache/impala/analysis/TableDef.java: http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/main/java/org/apache/impala/analysis/TableDef.java@56 PS2, Line 56: > nit: move this to the "org.apache" group and keep the sort order? Done http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/main/java/org/apache/impala/analysis/TableDef.java@551 PS2, Line 551: // We do not support ENABLE and VALIDATE. > Could you leave a comment about when will we throw this exception? This is not required anymore as I changed getPrimaryKeysSql to not make any RPCs. http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java: http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@305 PS2, Line 305: public static String getCreateTableSql(FeTable table) throws CatalogException { > Please correct me if I'm wrong. I feel like we don't do any external RPCs t You are right. It is unnecessary to make an RPC for SHOW CREATE statements. I modified the get*sql() methods to log a warning and return empty lists. SHOW CREATE is functionality is yet to be fully implemented. It can be tracked here in IMPALA-8290 http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java File fe/src/main/java/org/apache/impala/catalog/FeFsTable.java: http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java@19 PS2, Line 19: import java.util.ArrayList; > nit: move this to the "com.google" import group and keep the order? Done http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java@56 PS2, Line 56: > nit: move this above? Done http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java@190 PS2, Line 190: > MetaException extends TException. Do we really need it? We actually don't need it. Removed it. http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java@243 PS2, Line 243: h > nit: spaces around Done http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java@247 PS2, Line 247: return Boolean.parseBoolean(propVal); > Could you please add some comments about 'key_seq'? I feel a little confuse Done http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@801 PS2, Line 801: public Pair<List<SQLPrimaryKey>, List<SQLForeignKey>> loadConstraints( : final TableMetaRef table, Table msTbl) { > nit: take the same code style as other functions? Done http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@167 PS2, Line 167: public Pair<List<SQLPrimaryKey>, List<SQLForeignKey>> loadConstraints( : TableMetaRef table, Table msTbl) throws TException { > nit: take the same code style as other functions? Done http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java File fe/src/test/java/org/apache/impala/catalog/CatalogTest.java: http://gerrit.cloudera.org:8080/#/c/14731/2/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java@449 PS2, Line 449: List<SQLForeignKey> foreignKeys = t.getForeignKeys(); : assertEquals(2, primaryKeys.size()); > Should we also check the contents, e.g colum names and types, parent table Done. Added a bunch of other checks to this and other tests. -- To view, visit http://gerrit.cloudera.org:8080/14731 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ea7e1bacf6eb502c67caf310a847b32687e0d58 Gerrit-Change-Number: 14731 Gerrit-PatchSet: 3 Gerrit-Owner: Anurag Mantripragada <anu...@cloudera.com> Gerrit-Reviewer: Anurag Mantripragada <anu...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Tue, 19 Nov 2019 06:44:06 +0000 Gerrit-HasComments: Yes
