Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/16008 )
Change subject: IMPALA-9791: Support validWriteIdList in getPartialCatalogObject API ...................................................................... Patch Set 2: (8 comments) I did the first round, I will still have to go through the tests. http://gerrit.cloudera.org:8080/#/c/16008/2/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: http://gerrit.cloudera.org:8080/#/c/16008/2/common/thrift/CatalogService.thrift@497 PS2, Line 497: valid_write_id_list Nit: Could we use the same variable name for this field to avoid confusion in code? http://gerrit.cloudera.org:8080/#/c/16008/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/16008/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1851 PS2, Line 1851: Nit: Indent 4 spaces. http://gerrit.cloudera.org:8080/#/c/16008/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1868 PS2, Line 1868: Nit: Indent 4 spaces. http://gerrit.cloudera.org:8080/#/c/16008/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3027 PS2, Line 3027: String dbName = objectDesc.getTable().db_name == null ? "default" : : objectDesc.getTable().db_name; I could not find the code for it but does a similar conversion to "default" db happen in the later getOrLoadTable() call? I just want to make sure this conversion is consistent. http://gerrit.cloudera.org:8080/#/c/16008/2/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java File fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/16008/2/fe/src/main/java/org/apache/impala/catalog/FileMetadataLoader.java@196 PS2, Line 196: Nit: indent with 4 spaces. http://gerrit.cloudera.org:8080/#/c/16008/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/16008/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1536 PS2, Line 1536: LoadStats stats = new LoadStats(getHdfsBaseDirPath()); If LoadStats is only needed if `req.table_info_selector.want_partition_files` is true, could we move this initilization inside the if-statement on L1561 below? http://gerrit.cloudera.org:8080/#/c/16008/2/fe/src/main/java/org/apache/impala/util/AcidUtils.java File fe/src/main/java/org/apache/impala/util/AcidUtils.java: http://gerrit.cloudera.org:8080/#/c/16008/2/fe/src/main/java/org/apache/impala/util/AcidUtils.java@324 PS2, Line 324: public static List Could you add a method description? http://gerrit.cloudera.org:8080/#/c/16008/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/16008/2/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java@138 PS2, Line 138: Nit: 4 space indentation needed here and other places in this file. -- To view, visit http://gerrit.cloudera.org:8080/16008 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ied2c7c3cb2009c407e8fbc3af4722b0d34f57c4a Gerrit-Change-Number: 16008 Gerrit-PatchSet: 2 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 01 Jun 2020 22:33:58 +0000 Gerrit-HasComments: Yes
