Alex Behm has posted comments on this change. Change subject: IMPALA-4033,IMPALA-4105: Improvements of partition DDL. ......................................................................
Patch Set 1: (8 comments) Thanks for continuing this work! Can you please separate this patch into two ones, one for each JIRA? That generally makes it easier for us to do backports of bugfixes. http://gerrit.cloudera.org:8080/#/c/5137/1/bin/start-catalogd.sh File bin/start-catalogd.sh: Line 54: export JAVA_TOOL_OPTIONS="-agentlib:jdwp=transport=dt_socket,address=${JVM_DEBUG_PORT},server=y,suspend=${JVM_SUSPEND} ${JAVA_TOOL_OPTIONS}" what's the motivation for this change? http://gerrit.cloudera.org:8080/#/c/5137/1/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: Line 192: replace "some" with "the" http://gerrit.cloudera.org:8080/#/c/5137/1/fe/src/main/java/org/apache/impala/analysis/PartitionSet.java File fe/src/main/java/org/apache/impala/analysis/PartitionSet.java: Line 163 this fix needs a test http://gerrit.cloudera.org:8080/#/c/5137/1/fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java File fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java: Line 73 The toLowerCase() in this file seem correct to me, why did you remove them? http://gerrit.cloudera.org:8080/#/c/5137/1/fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java File fe/src/main/java/org/apache/impala/analysis/ResetMetadataStmt.java: Line 40: // not null when refreshing some partition(s) replace "some" with "a set of" Line 47: this.tableName_ = name; can remove 'this' everywhere in this constructor Line 74: TableRef tableRef = new TableRef(tableName_.toPath(), null, Privilege.ALTER); This is a pretty drastic behavioral change. Look at the comment on L62 following. We basically want to avoid loading the entire table metadata for such a refresh. However, we need the table metadata to fetch the partitions corresponding to the predicates. It's not really clear to me what we should do here, but regressing the existing behavior seems very dangerous. I'll need some time to think about this. Feel free to propose ideas :) http://gerrit.cloudera.org:8080/#/c/5137/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: Line 1256: hmsPartition = msClient.getHiveClient().getPartition( use the bulk getPartitions() API to get all partitions in one RPC -- To view, visit http://gerrit.cloudera.org:8080/5137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0f4e46ec0a63b46e485141290268d019c3dd15c7 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Amos Bird <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-HasComments: Yes
