Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11341 )
Change subject: IMPALA-7424: Reduce in-memory footprint of incremental stats ...................................................................... Patch Set 6: (7 comments) Looking pretty close, just a few small nits. http://gerrit.cloudera.org:8080/#/c/11341/6/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/11341/6/common/thrift/CatalogObjects.thrift@303 PS6, Line 303: required shouldn't this be optional since we don't need to send this to backends as part of descriptors? http://gerrit.cloudera.org:8080/#/c/11341/6/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java File fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java: http://gerrit.cloudera.org:8080/#/c/11341/6/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java@322 PS6, Line 322: part.hasIncrementalStats()); per comment elsewhere, I dont think you need this in the non-FULL (descriptor) object http://gerrit.cloudera.org:8080/#/c/11341/6/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/11341/6/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@32 PS6, Line 32: import com.google.common.base.Predicate; nit: this should sort down with the other guava stuff http://gerrit.cloudera.org:8080/#/c/11341/6/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@668 PS6, Line 668: // Delete the incremental stats entries. I think you could simplify this code a bit and write: Maps.filterKeys(partitionStats, IS_INCREMENTAL_STATS_KEY).clear(); (the doc for filterKeys says: > When methods such as removeAll() and clear() are called on the filtered map > or its views, only mappings whose keys satisfy the filter will be removed > from the underlying map. ) In Java 8, you could do partitionStats.keySet().removeIf(IS_INCREMENTAL_STATS_KEY) I think, but maybe you want to retain the option to backport this to Impala 2.x http://gerrit.cloudera.org:8080/#/c/11341/6/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@693 PS6, Line 693: hmsParamters_ nit: typo http://gerrit.cloudera.org:8080/#/c/11341/6/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/11341/6/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1712 PS6, Line 1712: part.hasIncrementalStats it's somewhat weird that this one is required. I would include it optionally, and only include it when the caller requests "want_partition_metadata" http://gerrit.cloudera.org:8080/#/c/11341/6/fe/src/main/java/org/apache/impala/util/CompressionUtil.java File fe/src/main/java/org/apache/impala/util/CompressionUtil.java: http://gerrit.cloudera.org:8080/#/c/11341/6/fe/src/main/java/org/apache/impala/util/CompressionUtil.java@43 PS6, Line 43: BEST_COMPRESSION curious if you compared the compression ratio and performance of BEST_COMPRESSION vs other options like DEFAULT and even HUFFMAN_ONLY? Maybe add a TODO here. -- To view, visit http://gerrit.cloudera.org:8080/11341 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39f02ebfa0c6e9b0baedd0d76058a1b34efb5a02 Gerrit-Change-Number: 11341 Gerrit-PatchSet: 6 Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 31 Aug 2018 04:13:46 +0000 Gerrit-HasComments: Yes
