Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/20010 )
Change subject: IMPALA-11996: Scanner change for Iceberg metadata querying ...................................................................... Patch Set 9: (13 comments) Nice patch Tamas! I left mostly nits, great job! http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/exec-node.cc File be/src/exec/exec-node.cc: http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/exec-node.cc@55 PS9, Line 55: #include "exec/iceberg-metadata/iceberg-metadata-scan-node.h" nit: could go to L41 http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h File be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h: http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@23 PS9, Line 23: #include <boost/scoped_ptr.hpp> Unused include http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.h@35 PS9, Line 35: /// its parquet scanner to scan Iceberg data, due to the predefined nature of the metadata nit: Parquet http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc File be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-metadata-scan-node.cc@41 PS9, Line 41: metadata_table_name_(pnode.tnode_->iceberg_scan_metadata_node.metadata_table_name) {}; nit: ; not needed http://gerrit.cloudera.org:8080/#/c/20010/9/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/20010/9/be/src/exec/iceberg-metadata/iceberg-row-reader.h@37 PS9, Line 37: IcebergRowReader(const TupleDescriptor* tuple_desc, jaccessors could be passed as a reference http://gerrit.cloudera.org:8080/#/c/20010/9/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/20010/9/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@128 PS9, Line 128: *reinterpret_cast<int64_t*>(slot) = reinterpret_cast<int32_t>(result); slot could be casted to int32_t* http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@181 PS9, Line 181: jstring result = (jstring)env->CallObjectMethod(accessed_value, char_sequence_value_); nit: static_cast may be a better approach here http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@186 PS9, Line 186: int str_len = strlen(str_guard.get()); Could be size_t http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/exec/iceberg-metadata/iceberg-row-reader.cc@190 PS9, Line 190: return tuple_data_pool->mem_tracker()->MemLimitExceeded(NULL, details, str_len); Could be nullptr http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/20010/9/be/src/service/frontend.cc@258 PS9, Line 258: Status Frontend::GetCatalogTable(const TTableName* table_name, jobject* resp) { GetCatalogTable could use the JniUtil::CallJniMethod function to fetch the object to keep these frontend calls in line. For this, a new ObjectToResult or Call overload is required. http://gerrit.cloudera.org:8080/#/c/20010/9/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/20010/9/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java@37 PS9, Line 37: * {IcebergMetadataScanNode} during scanning. nit: javadoc code references should be formatted as {@code text} http://gerrit.cloudera.org:8080/#/c/20010/9/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java@39 PS9, Line 39: * Iceberg generally drops RuntimeExceptions, these have to be taken care of by the caller nit: throws http://gerrit.cloudera.org:8080/#/c/20010/9/fe/src/main/java/org/apache/impala/util/IcebergMetadataScanner.java@43 PS9, Line 43: final static Logger LOG = LoggerFactory.getLogger(IcebergMetadataScanner.class); nit: private static final -- To view, visit http://gerrit.cloudera.org:8080/20010 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e943cecd77f5ef7af7cd07e2b596f2c5b4331e7 Gerrit-Change-Number: 20010 Gerrit-PatchSet: 9 Gerrit-Owner: Tamas Mate <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Gergely Fürnstáhl <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 06 Oct 2023 13:29:30 +0000 Gerrit-HasComments: Yes
