Daniel Becker 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 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/21125/8/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/21125/8/be/src/exec/iceberg-metadata/iceberg-metadata-scanner.cc@156 PS8, Line 156: jobject map_entry; > Shouldn't we release 'map_entry' at the end of this function? Done http://gerrit.cloudera.org:8080/#/c/21125/8/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/8/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@270 PS8, Line 270: DeleteLocalRef > Isn't 'collection_scanner' a GlobalRef? We call DeleteLocalRef here so I'm I checked the code and JNI doc again and I think it is actually a local ref and the comment was wrong. http://gerrit.cloudera.org:8080/#/c/21125/8/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@270 PS8, Line 270: env->DeleteLocalRef(collection_scanner); > I think we can leak memory if any of the RETURN_IF_ERROR or RETURN_IF_CANCE I checked the code and JNI doc again and I think it is actually a local ref and the comment was wrong. I updated the comments in iceberg-metadata-scanner.h. If the reference is indeed local, deleting it may not be as important. This is what the doc says about deleting local references (https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#:~:text=The%20JNI%20divides%20object%20references,until%20they%20are%20explicitly%20freed): In most cases, the programmer should rely on the VM to free all local references after the native method returns. However, there are times when the programmer should explicitly free a local reference. Consider, for example, the following situations: - A native method accesses a large Java object, thereby creating a local reference to the Java object. The native method then performs additional computation before returning to the caller. The local reference to the large Java object will prevent the object from being garbage collected, even if the object is no longer used in the remainder of the computation. - A native method creates a large number of local references, although not all of them are used at the same time. Since the VM needs a certain amount of space to keep track of a local reference, creating too many local references may cause the system to run out of memory. For example, a native method loops through a large array of objects, retrieves the elements as local references, and operates on one element at each iteration. After each iteration, the programmer no longer needs the local reference to the array element. I think Tamas added the deletes because of the second case. If an error occurs or the query is cancelled we won't create new (local) references, so freeing these local references is not important. If you'd like to I am open to creating a wrapper for these refs though. http://gerrit.cloudera.org:8080/#/c/21125/8/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@288 PS8, Line 288: env->DeleteLocalRef(item); > Same comment about leaking memory Done -- 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: 10 Gerrit-Owner: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 02 Apr 2024 12:36:22 +0000 Gerrit-HasComments: Yes
