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 3: (3 comments) 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; > I was under the assumption that since its a util method, it might be used b lets wait for others to comment. 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(); > Same as above, would we later need it in LocalFsTable? my thoughts exactly, lets wait for others to comment 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(); > I initially thought about it and then realized that it would skip the check you can probably add a check for unsupported formats -- 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: 3 Gerrit-Owner: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Pooja Nilangekar <[email protected]> Gerrit-Comment-Date: Mon, 23 Jul 2018 19:28:07 +0000 Gerrit-HasComments: Yes
