[Impala-ASF-CR] IMPALA-9791: Support validWriteIdList in getPartialCatalogObject API

["Aman Sinha (Code Review)" <ge...@cloudera.org>]

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 <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <an...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Jun 2020 21:52:33 +0000
Gerrit-HasComments: Yes