Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/21125 )
Change subject: IMPALA-12611: Add support to MAP type Iceberg Metadata table columns ...................................................................... Patch Set 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/21125/4/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/21125/4/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@63 PS4, Line 63: /// Note that it returns a GlobalRef, that has to be released explicitly. Probably I get this comment wrong, but I was searching for some code that releases whatever this function returns but didn't think anything relevant. http://gerrit.cloudera.org:8080/#/c/21125/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/21125/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@123 PS3, Line 123: VLOG(3) << "Skipping unsupported column type: " << slot_desc->type().type; Does this set Binary cols NULL now? http://gerrit.cloudera.org:8080/#/c/21125/4/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/21125/4/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@125 PS4, Line 125: env->DeleteLocalRef(accessed_value); I think this 'accessed_value' is allocated one level above in the call chain, so I'd make that level responsible for the deallocation. http://gerrit.cloudera.org:8080/#/c/21125/4/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@212 PS4, Line 212: if constexpr (IS_ARRAY) { Let's consider moving these DCHECKs into the same IF-ELSE at L228-234. http://gerrit.cloudera.org:8080/#/c/21125/4/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@220 PS4, Line 220: const TupleDescriptor* item_tuple_desc = slot_desc->children_tuple_descriptor(); nit: DCHECK item_tuple_desc is not null? http://gerrit.cloudera.org:8080/#/c/21125/4/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@236 PS4, Line 236: remaining_array_size name is misleading now that this is not just for arrays. remaining_items? http://gerrit.cloudera.org:8080/#/c/21125/4/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@297 PS4, Line 297: const jobject* key_struct_like_row = key_slot_desc->type().IsStructType() : ? &key : nullptr; Can the key of a map be a struct? http://gerrit.cloudera.org:8080/#/c/21125/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/21125/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-metadata-tables.test@761 PS3, Line 761: ==== > Added this query (except for "['spark.app.id']"). Can't we do a map_col.KEY and map_col.VALUE for maps? Would be nice to have some test coverage for these if possible. -- To view, visit http://gerrit.cloudera.org:8080/21125 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8a8b3a574ca45c893315c3b41b33ce4e0eff865a Gerrit-Change-Number: 21125 Gerrit-PatchSet: 5 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 28 Mar 2024 15:01:52 +0000 Gerrit-HasComments: Yes
