[Impala-ASF-CR] IMPALA-11996: Scanner change for Iceberg metadata querying

["Peter Rozsa (Code Review)" <ge...@cloudera.org>]

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 <tm...@apache.org>
Gerrit-Reviewer: Anonymous Coward <li...@apache.org>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <g....@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Oct 2023 13:29:30 +0000
Gerrit-HasComments: Yes