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: (14 comments) I managed to check the c++ part too 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@58 PS3, Line 58: jobject& result This should be pointer as Daniel said http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@93 PS3, Line 93: /// Accessor map for the scan result, pairs the slot ids with the java Accessor objects. This is not a map of accessors now but a slot ID to field ID mapping. http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@103 PS3, Line 103: Accessor collection I think this comment is off now. http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@109 PS3, Line 109: const SlotDescriptor* Don't we use const ref types for immutable params instead of const pointers? http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.h@113 PS3, Line 113: SlotDescriptor* const ref instead http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc File be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc: http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@88 PS3, Line 88: Status IcebergMetadataScanner::InitSlotIdFieldIdMap(JNIEnv* env) { Could you help me understand if this functiondoes or should go into collections, e.g. get the field ids for struct members inside collections? http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@145 PS3, Line 145: clazz I saw this param name elsewhere too, but not sure if this is something conventional or if it makes something, but seems a weird name for me. http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@155 PS3, Line 155: Primitive inside an ARRAY Can we DCHECK on this fact? http://gerrit.cloudera.org:8080/#/c/21061/3/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/3/be/src/exec/iceberg-metadata/iceberg-row-reader.h@22 PS3, Line 22: #include "exec/iceberg-metadata/iceberg-metadata-scanner.h" I think we can get rid of this include with a forward declaration. 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@114 PS3, Line 114: env->DeleteLocalRef(accessed_value); I'm a bit lost with these refs, but aren't local refs released automatically when going out from the scope? 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(), Could you add a test that exercises the path where EvalConjuncts is false? http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@237 PS3, Line 237: For nit: I'd fing 'From' instead of 'For' easier to understand http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@248 PS3, Line 248: } case TYPE_STRUCT: { Would it work to simply remove this case? http://gerrit.cloudera.org:8080/#/c/21061/3/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@252 PS3, Line 252: return list_cl_; Is this java.lang.util.List? Please add a comment similarly to the other types -- 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: Tue, 27 Feb 2024 16:43:07 +0000 Gerrit-HasComments: Yes
