Noemi Pap-Takacs has posted comments on this change. ( http://gerrit.cloudera.org:8080/23838 )
Change subject: IMPALA-14564: Remove redundant partition info from Iceberg file descriptors ...................................................................... Patch Set 3: (23 comments) Thanks for the comments! Posting this patch before the rebase to help track the changes. http://gerrit.cloudera.org:8080/#/c/23838/3/be/src/exec/file-metadata-utils.h File be/src/exec/file-metadata-utils.h: http://gerrit.cloudera.org:8080/#/c/23838/3/be/src/exec/file-metadata-utils.h@76 PS3, Line 76: /// Writes the Iceberg columns from FbIcebergMetadataRange into template_tuple. Updates the > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/23838/3/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/23838/3/be/src/exec/hdfs-scan-node-base.cc@269 PS3, Line 269: file_metadata = flatbuffers::GetRoot<org::apache::impala::fb::FbFileMetadataRange>( > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/23838/3/be/src/exec/hdfs-scan-node-base.cc@914 PS3, Line 914: const FbFileMetadataRange* file_metadata = context->GetStream(0)->file_desc()->file_metadata; > line too long (95 > 90) Done http://gerrit.cloudera.org:8080/#/c/23838/3/common/fbs/CatalogObjects.fbs File common/fbs/CatalogObjects.fbs: PS3: > This file is used in communication between impalad and catalogd/statestore. Ack http://gerrit.cloudera.org:8080/#/c/23838/3/common/fbs/IcebergObjects.fbs File common/fbs/IcebergObjects.fbs: http://gerrit.cloudera.org:8080/#/c/23838/3/common/fbs/IcebergObjects.fbs@55 PS3, Line 55: //spec_id : ushort; : //partition_keys : [FbIcebergPartitionTransformValue]; > This can be removed. Same goes to FbIcebergMetadataRange's comments. Done http://gerrit.cloudera.org:8080/#/c/23838/3/common/fbs/IcebergObjects.fbs@71 PS3, Line 71: -1 > Please comment why does it use a different default than FbIcebergMetadata.f Actually, both this and FbIcebergMetadata.first_row_id could use -1 as default value then for this reason. http://gerrit.cloudera.org:8080/#/c/23838/3/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/23838/3/common/thrift/CatalogObjects.thrift@681 PS3, Line 681: 9: optional list<binary> partitions > Changing type of field 'partitions' from list<TIcebergPartition> to list<bi Ack http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java File fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java: http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@206 PS3, Line 206: // Partitions with their corresponding ids that are used to refer to the partition info > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@286 PS3, Line 286: partition > Should we call normalizePartitionBuf? Or a Precondition check that partitio Done http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/catalog/IcebergDeleteTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergDeleteTable.java: http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/catalog/IcebergDeleteTable.java@37 PS3, Line 37: import com.google.common.base.Preconditions; > Unused import Done http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/catalog/IcebergDeleteTable.java@100 PS3, Line 100: > nit: too much indent Done http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java File fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@57 PS3, Line 57: import java.nio.ByteBuffer; > nit: misplaced import Done http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@82 PS3, Line 82: import java.nio.ByteBuffer; > nit: misplaced import Done http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/util/IcebergUtil.java File fe/src/main/java/org/apache/impala/util/IcebergUtil.java: http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1145 PS3, Line 1145: 1 > We can use the default constructor to minimize re-allocations. At the end o Done http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1155 PS3, Line 1155: > nit: indentation Done http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1160 PS3, Line 1160: compressedBb > nit: we usually name these as 'compressedBb' in other methods as well, but Done http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1164 PS3, Line 1164: public static FbFileMetadataRange transformFileMetadata(IcebergFileDescriptor fileDesc, > Please add a comment. Done http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1169 PS3, Line 1169: 1 > Same as above, we should use the default constructor. Done http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1175 PS3, Line 1175: FbIcebergMetadataRange.addFirstRowId(fbb, iceMetadata.firstRowId()); > line has trailing whitespace Done http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1185 PS3, Line 1185: return FbFileMetadataRange.getRootAsFbFileMetadataRange((ByteBuffer)compressedBb.flip()); > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1188 PS3, Line 1188: private static int copyPartitionKeys(FlatBufferBuilder fbb, FbIcebergPartition partition) { > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1205 PS3, Line 1205: partitionKeyOffsets[i] = > line has trailing whitespace Done http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/test/java/org/apache/impala/catalog/FileMetadataLoaderTest.java File fe/src/test/java/org/apache/impala/catalog/FileMetadataLoaderTest.java: http://gerrit.cloudera.org:8080/#/c/23838/3/fe/src/test/java/org/apache/impala/catalog/FileMetadataLoaderTest.java@36 PS3, Line 36: import java.nio.ByteBuffer; > nit: misplaced import Done -- To view, visit http://gerrit.cloudera.org:8080/23838 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I57c2fd6f1ebb636aa9e7ca925413ca51858cbc2a Gerrit-Change-Number: 23838 Gerrit-PatchSet: 3 Gerrit-Owner: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 04 May 2026 12:41:31 +0000 Gerrit-HasComments: Yes
