Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/20010 )
Change subject: IMPALA-11996: Scanner change for Iceberg metadata querying ...................................................................... Patch Set 14: (8 comments) Thank you for the review Zoltan, updated the change. 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 <jni.h> > Seems like we can remove this. Done 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: } : } : return Status::OK(); : } : : Status IcebergRowReader::WriteBooleanSlot(JNIEnv* env, : > These lines are common in all Read methods. It might make sense to put them I moved it to MaterializeRow and renamed the Read methods. http://gerrit.cloudera.org:8080/#/c/20010/13/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@111 PS13, Line 111: java_boolean_c > Can we check somehow that accessed_value has the expected type? I could DCHECK an IsInstanceOf call. 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? This is for the scheduler, to execute the fragment on the coordinator. scheduler.cc:171 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: * Creates the Metadata{Table} which is a predifined Iceberg {Table} object. This method : * also starts an Iceberg {TableScan} to scan the {Table}. After the scan is ready it : * initializes the iterators, so the {GetNext} call can start fetching the rows through : * the Iceberg Api. : */ > nit: Almost the same as L102-L106. Can we refactor this? Done 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: metadata t > nit: metadata Done 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 metadata tables once > Is there a list somewhere about all the metadata tables? Only in the Iceberg codebase at the moment. https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/MetadataTableType.java 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 metad > nit: the metadata tables Done -- 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: 14 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: Thu, 19 Oct 2023 12:55:01 +0000 Gerrit-HasComments: Yes
