Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21141 )
Change subject: WIP IMPALA-12896: Avoid JDBC table to be set as transactional table ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/21141/1/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java: http://gerrit.cloudera.org:8080/#/c/21141/1/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@809 PS1, Line 809: Exception Why this is widen from only catching IOException to all Exception? http://gerrit.cloudera.org:8080/#/c/21141/1/fe/src/main/java/org/apache/impala/util/MaxRowsProcessedVisitor.java File fe/src/main/java/org/apache/impala/util/MaxRowsProcessedVisitor.java: http://gerrit.cloudera.org:8080/#/c/21141/1/fe/src/main/java/org/apache/impala/util/MaxRowsProcessedVisitor.java@36 PS1, Line 36: // Max number of rows processed across all instances of a plan node. : private long maxRowsProcessed_ ; : : // Max number of rows processed per backend impala daemon for a plan node. : private long maxRowsProcessedPerNode_; This a good chance to explicitly init these to 0. http://gerrit.cloudera.org:8080/#/c/21141/1/fe/src/main/java/org/apache/impala/util/MaxRowsProcessedVisitor.java@49 PS1, Line 49: // Operations on DataSourceScanNode are processed on coordinator. : maxRowsProcessed_ = Math.max(maxRowsProcessed_, 0); : maxRowsProcessedPerNode_ = Math.max(maxRowsProcessedPerNode_, 0); Is this basically intended to be a no-op? If yes, probably better to leave it empty and just add Preconditions that numNodes == 1. Just like the very last else branch, this should check if caller is valid and return early when it is invalid. if ((in == -1) || (out == -1)) { valid_ = false; return; } At the end of this method, you can add instead add Preconditions that verify both maxRowsProcessed_ and maxRowsProcessedPerNode_ are always >= 0 after each visit. http://gerrit.cloudera.org:8080/#/c/21141/1/tests/custom_cluster/test_ext_data_sources.py File tests/custom_cluster/test_ext_data_sources.py: http://gerrit.cloudera.org:8080/#/c/21141/1/tests/custom_cluster/test_ext_data_sources.py@51 PS1, Line 51: vector.get_value('exec_option') Use deepcopy http://gerrit.cloudera.org:8080/#/c/21141/1/tests/custom_cluster/test_ext_data_sources.py@254 PS1, Line 254: vector.get_value('exec_option') Use deepcopy -- To view, visit http://gerrit.cloudera.org:8080/21141 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I556faeda923a4a11d4bef8c1250c9616f77e6fa6 Gerrit-Change-Number: 21141 Gerrit-PatchSet: 1 Gerrit-Owner: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Tue, 12 Mar 2024 19:25:00 +0000 Gerrit-HasComments: Yes
