Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/16512 )
Change subject: IMPALA-10187: Add PARTITON BY SPEC to SHOW CREATE TABLE for Iceberg Tables ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/16512/1/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionField.java File fe/src/main/java/org/apache/impala/analysis/IcebergPartitionField.java: http://gerrit.cloudera.org:8080/#/c/16512/1/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionField.java@79 PS1, Line 79: // Iceberg library's PartitionSpec.Builder adds a postfix to the field name that : // represents the transform type of that field. E.g. "partition_name Day" would be : // named partition_name_day. This function returns the field name without that postfix. : private String fieldNameWithoutPostfix() { : String transformPostfix = "_" + fieldType_.toString(); : if (fieldName_.toUpperCase().endsWith(transformPostfix)) { : return fieldName_.substring(0, fieldName_.length() - transformPostfix.length()); : } : return fieldName_; : } > In fact, I think get PartitionField related column name by truncate 'fieldN I'm not sure I understand the comment, but it's not enough to save the truncated field name for later use as this should also work for field names coming from Iceberg. http://gerrit.cloudera.org:8080/#/c/16512/1/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergTable.java: http://gerrit.cloudera.org:8080/#/c/16512/1/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@244 PS1, Line 244: defaultPartitionSpecId_ = metadata.defaultSpecId() > Yeah, I think we should use the same vocabulary that Iceberg uses. I found the naming in Iceberg incorrect, as this is not the default spec but the latest one. But I agree to keep the naming consistent. Renaming this to defaultPartitionSpecId_ http://gerrit.cloudera.org:8080/#/c/16512/1/fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java: http://gerrit.cloudera.org:8080/#/c/16512/1/fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java@115 PS1, Line 115: @Override : public int getDefaultPartitionSpecId() { return defaultPartitionSpecId_; } : : @Override : public IcebergPartitionSpec getDefaultPartitionSpec() { : return Utils.getDefaultPartitionSpec(this); : } > It seems that this method is same as IcebergTable.getLatestPartitionSpec(). Good point. Done and done. -- To view, visit http://gerrit.cloudera.org:8080/16512 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie4c43b75057807ab513a220d348155be2487e714 Gerrit-Change-Number: 16512 Gerrit-PatchSet: 2 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: wangsheng <[email protected]> Gerrit-Comment-Date: Tue, 29 Sep 2020 14:52:56 +0000 Gerrit-HasComments: Yes
