Sai Hemanth Gantasala has posted comments on this change. ( http://gerrit.cloudera.org:8080/21437 )
Change subject: IMPALA-12277: Fix NullPointerException for partitioned inserts when partition list is stale ...................................................................... Patch Set 17: (9 comments) http://gerrit.cloudera.org:8080/#/c/21437/16/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/16/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1293 PS16, Line 1293: > nit: can we rename this to 'getMsClient()' ? It's confused whether it's an Ack http://gerrit.cloudera.org:8080/#/c/21437/16/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1296 PS16, Line 1296: > nit: instead of calling this multiple times, we can replace it with a local Ack http://gerrit.cloudera.org:8080/#/c/21437/16/fe/src/main/java/org/apache/impala/catalog/HdfsTableLoadParams.java File fe/src/main/java/org/apache/impala/catalog/HdfsTableLoadParams.java: http://gerrit.cloudera.org:8080/#/c/21437/16/fe/src/main/java/org/apache/impala/catalog/HdfsTableLoadParams.java@32 PS16, Line 32: pr > nit: use 2 spaces indention Ack http://gerrit.cloudera.org:8080/#/c/21437/16/fe/src/main/java/org/apache/impala/catalog/HdfsTableLoadParamsBuilder.java File fe/src/main/java/org/apache/impala/catalog/HdfsTableLoadParamsBuilder.java: http://gerrit.cloudera.org:8080/#/c/21437/16/fe/src/main/java/org/apache/impala/catalog/HdfsTableLoadParamsBuilder.java@30 PS16, Line 30: bo > nit: use 2 spaces indention Ack http://gerrit.cloudera.org:8080/#/c/21437/16/fe/src/main/java/org/apache/impala/catalog/HdfsTableLoadParamsBuilder.java@62 PS16, Line 62: > nit: use 'reuseMetadata' might be more clear Ack http://gerrit.cloudera.org:8080/#/c/21437/16/fe/src/main/java/org/apache/impala/catalog/HdfsTableLoadParamsBuilder.java@67 PS16, Line 67: > nit: 'setMsClient' might be more clear Ack http://gerrit.cloudera.org:8080/#/c/21437/16/fe/src/main/java/org/apache/impala/catalog/HdfsTableLoadParamsBuilder.java@77 PS16, Line 77: > nit: use a better name, e.g. setLoadPartitionFileMetadata? Ack 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@7279 PS13, Line 7279: he createEventId for the partitions. > Not done yet. Can we use a better method name? Is the new name any better? http://gerrit.cloudera.org:8080/#/c/21437/16/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/16/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@7260 PS16, Line 7260: ta(true).setL > Why need a new HashSet instance here? I wanted to pass by the value 'partsToCreate' so that the original value of this is intact for future reference down in the method. Regarding setters, thanks for pointing it out, I originally intended to use setters introduced in the HdfsTableLoadParamsBuilder class but forgot about it completely. -- 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: 17 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: Wed, 10 Jul 2024 01:05:29 +0000 Gerrit-HasComments: Yes
