Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/21061 )
Change subject: IMPALA-12610: Support reading ARRAY columns for Iceberg Metadata tables ...................................................................... Patch Set 3: (9 comments) Thanks for the patch, Tamas! I checked the tests and the Java part. Will take a deeper look at the rest tomorrow. http://gerrit.cloudera.org:8080/#/c/21061/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21061/3//COMMIT_MSG@18 PS3, Line 18: the the nit: duplicate 'the' http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h File be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h: http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@21 PS3, Line 21: #include "exec/scan-node.h" Can we not include scan-node.h? http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/service/impalad-main.cc File be/src/service/impalad-main.cc: http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/service/impalad-main.cc@32 PS3, Line 32: #include "exec/iceberg-metadata/iceberg-metadata-scan-node.h" This include can be removed I think http://gerrit.cloudera.org:8080/#/c/21061/3/fe/src/main/java/org/apache/impala/analysis/FromClause.java File fe/src/main/java/org/apache/impala/analysis/FromClause.java: http://gerrit.cloudera.org:8080/#/c/21061/3/fe/src/main/java/org/apache/impala/analysis/FromClause.java@169 PS3, Line 169: tblRef.getDesc().getType().isMapType()) { If I'm not mistaken, this check is for the case when a collection is provided in the FROM clause. If this use case is deliberately supported then please add test coverage too. http://gerrit.cloudera.org:8080/#/c/21061/3/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/21061/3/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java@116 PS3, Line 116: StructLke typo http://gerrit.cloudera.org:8080/#/c/21061/3/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java@137 PS3, Line 137: private Iterator<T> iterator; If we store the iterator as a member, is there a guarantee that the underlying array is not destroyed? http://gerrit.cloudera.org:8080/#/c/21061/3/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java@146 PS3, Line 146: return iterator.next(); nit: could fit into the line above http://gerrit.cloudera.org:8080/#/c/21061/3/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/21061/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@567 PS3, Line 567: # Test 11 : Query ARRAY type columns The test above is also "Test 11". I don't think that giving a number of these tests make sense, it just makes it complicated to maintain and to add anything to the middle. http://gerrit.cloudera.org:8080/#/c/21061/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@591 PS3, Line 591: row_regex:'/test-warehouse/iceberg_test/hadoop_catalog/ice/iceberg_v2_delete_both_eq_and_pos/data/.*','NULL' The name of the files in this table is fix, so we could spell the full path out in the expected results, in my opinion. -- To view, visit http://gerrit.cloudera.org:8080/21061 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieb9bac1822a17bd3cd074b4b98e4d010703cecb1 Gerrit-Change-Number: 21061 Gerrit-PatchSet: 3 Gerrit-Owner: Tamas Mate <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 26 Feb 2024 16:22:30 +0000 Gerrit-HasComments: Yes
