Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/22605 )
Change subject: IMPALA-13738 (Part1): Refactor IcebergTable.getPartialInfo() ...................................................................... Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/22605/4/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/22605/4/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java@283 PS4, Line 283: isAcid > I'd suggest "isHiveAcid()". It's unlikely, but we may introduce other ACID +1, let's keep it as explicit as we can, especially because ACID is a quite common term and can be applicable to other table formats e.g. Iceberg/Hudi. http://gerrit.cloudera.org:8080/#/c/22605/4/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java: http://gerrit.cloudera.org:8080/#/c/22605/4/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@618 PS4, Line 618: partInfo.hms_parameters = getParameters(); nit: Originally this line had the following comment: // Don't need to make a copy here since the caller should not modify the parameters. It think it's still valid to mention why we don't have to make a deep copy of HMS parameters, e.g.: // Don't need to make a copy here since the HMS parameters shouldn't change during the invocation of getPartitionInfo. http://gerrit.cloudera.org:8080/#/c/22605/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/22605/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@464 PS4, Line 464: public boolean isAcid() { It could quickly return by checking validWriteIds_ != null; To make it more robust it could: if (validWriteIds_ != null) return true; return AcidUtils.isTransactionalTable(msTable_.getParameters()); This way it would also work if validWriteIds_ is not yet initialized for some reason. http://gerrit.cloudera.org:8080/#/c/22605/3/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/22605/3/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@798 PS3, Line 798: // of cloning of file descriptors which might increase memory pressure. > I'll leave this up to you though...but I think it makes sense to do this no My two cents: I like that this patch focuses on partition info, it makes it easier to oversee this refactoring. I'm afraid that doing more and more in this CR will make it too complex. So I'd suggest to do this step (or the introduction of FsTable) in another CR. http://gerrit.cloudera.org:8080/#/c/22605/4/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/22605/4/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@813 PS4, Line 813: resp.table_info.unsetVirtual_columns(); I think this line is not needed anymore as hdfsTable_ won't set any virtual columns from now. -- To view, visit http://gerrit.cloudera.org:8080/22605 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75d277fb555d0088a2e28111ff529a4605d734fa Gerrit-Change-Number: 22605 Gerrit-PatchSet: 4 Gerrit-Owner: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Steve Carlin <scar...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 25 Mar 2025 15:55:12 +0000 Gerrit-HasComments: Yes
