Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21437 )
Change subject: IMPALA-12277: Fix NullPointerException for partitioned inserts when EP is turned off ...................................................................... Patch Set 13: (7 comments) Thanks for refactoring the long methods! Just took a first pass. I'll look deeper into this. http://gerrit.cloudera.org:8080/#/c/21437/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21437/14//COMMIT_MSG@7 PS14, Line 7: when : EP is turned off nit: let's make this accurate, e.g. "when partition list is stale" http://gerrit.cloudera.org:8080/#/c/21437/13/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/21437/13/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1292 PS13, Line 1292: Preconditions.checkState( nit: as we touch this code, can we add an error message for Preconditions.checkState() BTW? E.g. Preconditions.checkState( partitionsToUpdate == null || loadPartitionFileMetadata, "Conflicts in 'partitionsToUpdate' and 'loadPartitionFileMetadata'"); http://gerrit.cloudera.org:8080/#/c/21437/14/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/21437/14/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1238 PS14, Line 1238: metadata should be updated. O nit: we usually put the comment before the value, e.g. L1225-L1227 http://gerrit.cloudera.org:8080/#/c/21437/13/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/21437/13/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@7253 PS13, Line 7253: false nit: could you add a comment to highlight this is 'loadPartitionFileMetadata'? http://gerrit.cloudera.org:8080/#/c/21437/13/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@7279 PS13, Line 7279: updatePartitionsToCreateAndUnsetStats nit: let's avoid using the variable name 'partitionsToCreate' in the method name. Maybe 'pickupExistingPartitions' is enough. Other details like unsetting COLUMN_STATS_ACCURATE and collecting cacheDirIds can be mentioned in the method comments. http://gerrit.cloudera.org:8080/#/c/21437/13/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@7405 PS13, Line 7405: nit: remove one space here http://gerrit.cloudera.org:8080/#/c/21437/13/tests/custom_cluster/test_events_custom_configs.py File tests/custom_cluster/test_events_custom_configs.py: http://gerrit.cloudera.org:8080/#/c/21437/13/tests/custom_cluster/test_events_custom_configs.py@1275 PS13, Line 1275: self.client.execute("insert into {}.{} partition(year=2024) values (0)" Can we also add year=2022 as an existing partition? Now we have stale partition (year=2024) dropped externally and missing partition (year=2023) added externally. Just miss the case of inserting into existing partitions. -- To view, visit http://gerrit.cloudera.org:8080/21437 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ide8f1f6bf017e9a040b53bb5d5291ff2ea3e0d18 Gerrit-Change-Number: 21437 Gerrit-PatchSet: 13 Gerrit-Owner: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Fri, 14 Jun 2024 08:59:19 +0000 Gerrit-HasComments: Yes
