wangsheng 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 1: (3 comments) Hi Gabor, thanks for working on this patch. Here is some of my suggestions. 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 'fieldName_' maybe not very good. Maybe add another field in IcebergPartitionField to record coulmn name is better. In this case, even if Iceberg change the PartitionField naming rule in the future, we can still use these code. How do you think? 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: latestPartitionSpecId_ = metadata.defaultSpecId(); Can we have more comments here? The field name is 'latestPartitionSpecId_', but we get this value by defaultSpecId() method, this maybe confusing. 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 IcebergPartitionSpec getLatestPartitionSpec() { : Preconditions.checkState(partitionSpecs_ != null); : if (partitionSpecs_.isEmpty()) return null; : Preconditions.checkState(partitionSpecs_.size() > latestPartitionSpecId_); : return partitionSpecs_.get(latestPartitionSpecId_); : } It seems that this method is same as IcebergTable.getLatestPartitionSpec(). Can we implement this method in FeIcebergTable.Utils? So we modify this method like this: public IcebergPartitionSpec getLatestPartitionSpec() { return FeIcebergTable.Utils.getLatestPartitionSpec(this); } Besides, maybe we should provide a public function to get latestPartitionSpecId_, this is helpful for time travel function. -- 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: 1 Gerrit-Owner: 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 06:36:57 +0000 Gerrit-HasComments: Yes
