Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/16008 )
Change subject: IMPALA-9791: Support validWriteIdList in getPartialCatalogObject API ...................................................................... Patch Set 2: (5 comments) I did a first pass of the code changes. Haven't looked at the tests in detail. 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: 3: optional CatalogObjects.TValidWriteIdList valid_write_id_list May be better to keep the naming consistent i.e valid_write_ids (as defined in CatalogService.thrift) or make the other one same as this. 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@3026 PS2, Line 3026: if (req.table_info_selector.valid_write_ids != null) { This code block is for both TABLE and VIEW catalog objects. I would expect the ValidWriteIdList to be null for views .. or does it reflect a concatenation of the valid writeIds of all the tables contained in that view ? 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<FileDescriptor> filterFDsforAcidState(List<FileDescriptor> fds, Would be good to add a brief comment for the motivation for this method compared to the pre-existing filterFilesForAcidState. At first glance, it is not obvious why a second method was needed. Since you already give the motivation in WriteListBasedPredicate maybe you can point to those comments. http://gerrit.cloudera.org:8080/#/c/16008/2/fe/src/main/java/org/apache/impala/util/AcidUtils.java@329 PS2, Line 329: for (FileDescriptor fd : fds) { These file descriptors are the total across all (pruned) partitions, right? I think you have mentioned offline that partition level ValidWriteIdList could potentially be used to skip although currently they are not reliable. The performance would be an issue for very large lists where partition pruning was not too selective..but I suppose it is no worse than the existing filterFilesForAcidState() method. Maybe a future enhancement. http://gerrit.cloudera.org:8080/#/c/16008/2/fe/src/main/java/org/apache/impala/util/AcidUtils.java@579 PS2, Line 579: // if tbl is not a transactional, there is not to compare against and we return 0 nit: change 'not to compare ..' to 'nothing to compare ..' -- 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 21:52:33 +0000 Gerrit-HasComments: Yes
