Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20010 )
Change subject: IMPALA-11996: Scanner change for Iceberg metadata querying ...................................................................... Patch Set 13: (9 comments) Left some small comments, otherwise the code looks great! http://gerrit.cloudera.org:8080/#/c/20010/9/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/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@99 PS9, Line 99: std::unique_ptr > It makes managing the object easier. But I might misunderstood the the ques OK, I see you initialize it in Open(). http://gerrit.cloudera.org:8080/#/c/20010/13/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/20010/13/be/src/exec/iceberg-metadata/iceberg-row-reader.h@20 PS13, Line 20: // #include "exec/scan-node.h" Seems like we can remove this. http://gerrit.cloudera.org:8080/#/c/20010/13/be/src/exec/iceberg-metadata/iceberg-row-reader.cc File be/src/exec/iceberg-metadata/iceberg-row-reader.cc: http://gerrit.cloudera.org:8080/#/c/20010/13/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@104 PS13, Line 104: jobject accessed_value = env->CallObjectMethod(jaccessors_->at(slot_desc->col_pos()), : iceberg_accessor_get_, struct_like_row); : RETURN_ERROR_IF_EXC(env); : if (accessed_value == nullptr) { : tuple->SetNull(slot_desc->null_indicator_offset()); : return Status::OK(); : } These lines are common in all Read methods. It might make sense to put them into a helper function. http://gerrit.cloudera.org:8080/#/c/20010/13/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@111 PS13, Line 111: accessed_value Can we check somehow that accessed_value has the expected type? http://gerrit.cloudera.org:8080/#/c/20010/13/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java: http://gerrit.cloudera.org:8080/#/c/20010/13/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@414 PS13, Line 414: UNPARTITIONED Why are we using UNPARTITIONED instead of RANDOM? http://gerrit.cloudera.org:8080/#/c/20010/13/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java File fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java: http://gerrit.cloudera.org:8080/#/c/20010/13/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java@76 PS13, Line 76: while (fileScanTaskIterator_.hasNext()) { : DataTask dataTask = (DataTask)fileScanTaskIterator_.next(); : dataRowsIterator_ = dataTask.rows().iterator(); : if (dataRowsIterator_.hasNext()) break; : } nit: Almost the same as L102-L106. Can we refactor this? http://gerrit.cloudera.org:8080/#/c/20010/13/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/20010/13/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@6 PS13, Line 6: Query all the metadatata tables once Is there a list somewhere about all the metadata tables? http://gerrit.cloudera.org:8080/#/c/20010/13/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@6 PS13, Line 6: metadatata nit: metadata http://gerrit.cloudera.org:8080/#/c/20010/13/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/20010/13/tests/authorization/test_ranger.py@2048 PS13, Line 2048: the table nit: the metadata tables -- To view, visit http://gerrit.cloudera.org:8080/20010 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e943cecd77f5ef7af7cd07e2b596f2c5b4331e7 Gerrit-Change-Number: 20010 Gerrit-PatchSet: 13 Gerrit-Owner: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Anonymous Coward <lipeng...@apache.org> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Gergely Fürnstáhl <g.furnst...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Wed, 18 Oct 2023 15:16:31 +0000 Gerrit-HasComments: Yes
