Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/11001 )
Change subject: IMPALA-7234: Improve memory estimates produced by the Planner ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/11001/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11001/1//COMMIT_MSG@12 PS1, Line 12: calcuated nit: typo http://gerrit.cloudera.org:8080/#/c/11001/1//COMMIT_MSG@14 PS1, Line 14: esimate nit: typo http://gerrit.cloudera.org:8080/#/c/11001/1//COMMIT_MSG@18 PS1, Line 18: esimate nit: typo http://gerrit.cloudera.org:8080/#/c/11001/1/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/11001/1/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java@300 PS1, Line 300: public static Set<HdfsFileFormat> getFileFormats( : Iterable<? extends FeFsPartition> partitions) { : Set<HdfsFileFormat> fileFormats = Sets.newHashSet(); : for (FeFsPartition partition : partitions) { : fileFormats.add(partition.getFileFormat()); : } : return fileFormats; you can get rid of this since its used only in one place and move functionality to HdfsTable http://gerrit.cloudera.org:8080/#/c/11001/1/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/11001/1/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java@97 PS1, Line 97: /** : * @return the set of file formats that the partitions in this table use. : * This API is only used by the TableSink to write out partitions. It : * should not be used for scanning. : */ : public Set<HdfsFileFormat> getFileFormats(); we can probably get rid of this since its used at only one place and move the functionality there. Interested in what others think about this. http://gerrit.cloudera.org:8080/#/c/11001/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/11001/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1369 PS1, Line 1369: Henece nit: typo http://gerrit.cloudera.org:8080/#/c/11001/1/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java File fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java: http://gerrit.cloudera.org:8080/#/c/11001/1/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java@83 PS1, Line 83: : HdfsTable table = (HdfsTable) targetTable_; : // TODO: Estimate the memory requirements more accurately by partition type. : Set<HdfsFileFormat> formats = table.getFileFormats(); you can pass the table directly and also simplify getPerPartitionMemReq() by using a contains(FORMAT) instead of a for-loop + switch-case. Also add corresponding comments. http://gerrit.cloudera.org:8080/#/c/11001/1/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java@109 PS1, Line 109: * Returns the per-partition memory requirement for inserting into the given : * file format. update comment -- To view, visit http://gerrit.cloudera.org:8080/11001 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0666ae3d45fbd8615d3fa9a8626ebd29cf94fb4b Gerrit-Change-Number: 11001 Gerrit-PatchSet: 1 Gerrit-Owner: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Pooja Nilangekar <[email protected]> Gerrit-Comment-Date: Sat, 21 Jul 2018 01:13:02 +0000 Gerrit-HasComments: Yes
