Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20759 )
Change subject: IMPALA-12205: Add support to STRUCT type Iceberg Metadata table columns ...................................................................... Patch Set 2: (7 comments) Left some minor comments, but the code looks good to me. http://gerrit.cloudera.org:8080/#/c/20759/2/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h File be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h: http://gerrit.cloudera.org:8080/#/c/20759/2/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@126 PS2, Line 126: Recusrive Recursive http://gerrit.cloudera.org:8080/#/c/20759/2/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@130 PS2, Line 130: inot into http://gerrit.cloudera.org:8080/#/c/20759/2/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc File be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/20759/2/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc@87 PS2, Line 87: // Create field accessors : for (SlotDescriptor* slot_desc: tuple_desc_->slots()) { : if (slot_desc->type().IsStructType()) { : // Recursively get the field ids for struct fields : RETURN_IF_ERROR(CollectStructFieldAccessors(env, slot_desc)); : } else if (slot_desc->col_path().size() > 1) { : // Slot that is child of a struct without tuple, can occur when a struct member is : // in the select list : int root_type_index = slot_desc->col_path()[0]; : ColumnType current_type = : tuple_desc_->table_desc()->col_descs()[root_type_index].type(); : for (int i = 1; i < slot_desc->col_path().size() - 1; ++i) { : current_type = current_type.children[slot_desc->col_path()[i]]; : } : int field_id = current_type.field_ids[slot_desc->col_path().back()]; : RETURN_IF_ERROR(AddAccessorForFieldId(env, field_id, slot_desc->id())); : } else { : // For primitive types outside STRUCT use the ColumnDescriptors : int field_id = tuple_desc_->table_desc()->GetColumnDesc(slot_desc).field_id(); : RETURN_IF_ERROR(AddAccessorForFieldId(env, field_id, slot_desc->id())); : } : } nit: this could be moved to its own method. http://gerrit.cloudera.org:8080/#/c/20759/2/be/src/exec/iceberg-metadata/iceberg-row-reader.h File be/src/exec/iceberg-metadata/iceberg-row-reader.h: http://gerrit.cloudera.org:8080/#/c/20759/2/be/src/exec/iceberg-metadata/iceberg-row-reader.h@44 PS2, Line 44: Materlilize Materialize http://gerrit.cloudera.org:8080/#/c/20759/2/fe/src/main/java/org/apache/impala/analysis/SlotRef.java File fe/src/main/java/org/apache/impala/analysis/SlotRef.java: http://gerrit.cloudera.org:8080/#/c/20759/2/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@230 PS2, Line 230: rootTable instanceof IcebergMetadataTable) return; : if (!(rootTable instanceof FeFsTable nit: we could have a helper method for readability like 'TableTypeSupportsStruct(FeTable tbl)' http://gerrit.cloudera.org:8080/#/c/20759/2/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java: http://gerrit.cloudera.org:8080/#/c/20759/2/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergMetadataTable.java@73 PS2, Line 73: "Adding column: \"" + col.getName() + "\" with type: \"" + col.getType() + : "\" to metadata table." LOG.trace() supports formatting with placeholders {}. http://gerrit.cloudera.org:8080/#/c/20759/2/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test: http://gerrit.cloudera.org:8080/#/c/20759/2/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@496 PS2, Line 496: ==== Can we also add a SELECT * query with EXPAND_COMPLEX_TYPES=true? -- To view, visit http://gerrit.cloudera.org:8080/20759 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I953ad7253b270f2855bfcaee4ad023d1c4469273 Gerrit-Change-Number: 20759 Gerrit-PatchSet: 2 Gerrit-Owner: Tamas Mate <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 07 Dec 2023 12:27:26 +0000 Gerrit-HasComments: Yes
