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 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/21061/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21061/6//COMMIT_MSG@18 PS6, Line 18: alue nit: value http://gerrit.cloudera.org:8080/#/c/21061/6/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/21061/6/be/src/exec/iceberg-metadata/iceberg-row-reader.h@21 PS6, Line 21: #include "exec/scan-node.h" I think even including scan-node.h in the header could be avoided by forward declarations http://gerrit.cloudera.org:8080/#/c/21061/3/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/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@222 PS3, Line 222: if (ExecNode::EvalConjuncts(scan_node_->conjunct_evals().data(), > I just realised that with the current implementation it won't be possible t well, if this code doesn't work then let's remove it. What would be needed to make this work? FROM clause collections, and predicates on the ITEM, right? http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@248 PS3, Line 248: } case TYPE_STRUCT: { > Yes, but I think it helps to understand what is happening here. But then we return nullptr for TYPE_STRUCT that could cause issues on the caller side. If this method is not expected to be called for TYPE_STRUCT then it's cleaner to get rid of that case. 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: throw new AnalysisException("Querying collection types (ARRAY/MAP) in FROM " + > Adding Jira: IMPALA-12853 Thanks for creating a Jira! Could you also mention the Jira ID here in a comment? http://gerrit.cloudera.org:8080/#/c/21061/6/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/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@624 PS6, Line 624: '/test-warehouse/iceberg_test/hadoop_catalog/ice/iceberg_v2_delete_both_eq_and_pos/data/delete-074a9e19e61b766e-652a169e00000001_800513971_data.0.parq','NULL' Could you also add the content col to the select list so that we can see which files are eq-delete files. Would make it easier to verify that only eq-delete files (content=2) have eq-id list http://gerrit.cloudera.org:8080/#/c/21061/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@663 PS6, Line 663: select null_value_counts from functional_parquet.iceberg_query_metadata.all_files; This seems a duplicate test with the one on L715 http://gerrit.cloudera.org:8080/#/c/21061/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@715 PS6, Line 715: select null_value_counts from functional_parquet.iceberg_query_metadata.all_files; Shouldn't we get a not supported error when querying map columns from a metadata table? Returning nulls might be misleading -- 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: 6 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: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 29 Feb 2024 11:33:24 +0000 Gerrit-HasComments: Yes
