Quanlong Huang 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 16: (10 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: getClient nit: can we rename this to 'getMsClient()' ? It's confused whether it's an HDFS client or HMS client. http://gerrit.cloudera.org:8080/#/c/21437/16/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1296 PS16, Line 1296: loadParams.getClient() nit: instead of calling this multiple times, we can replace it with a local variable to save some code changes. E.g. add these at the beginning of the method: IMetaStoreClient client = loadParams.getMsClient(); org.apache.hadoop.hive.metastore.api.Table msTbl = loadParams.getMsTable(); EventSequence catalogTimeline = loadParams.getCatalogTimeline(); 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: nit: use 2 spaces indention 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: nit: use 2 spaces indention http://gerrit.cloudera.org:8080/#/c/21437/16/fe/src/main/java/org/apache/impala/catalog/HdfsTableLoadParamsBuilder.java@43 PS16, Line 43: public HdfsTableLoadParamsBuilder(boolean reuseMetadata, IMetaStoreClient client, A default constructor with only required arguments will be helpful. All other fields are set using the default values. E.g. client, msTbl seems to be the required arguments (i.e. used in all call sites). Then we need a constructor of public HdfsTableLoadParamsBuilder(IMetaStoreClient client, Table msTbl) http://gerrit.cloudera.org:8080/#/c/21437/16/fe/src/main/java/org/apache/impala/catalog/HdfsTableLoadParamsBuilder.java@62 PS16, Line 62: setIsReuseMetadata nit: use 'reuseMetadata' might be more clear http://gerrit.cloudera.org:8080/#/c/21437/16/fe/src/main/java/org/apache/impala/catalog/HdfsTableLoadParamsBuilder.java@67 PS16, Line 67: setClient nit: 'setMsClient' might be more clear http://gerrit.cloudera.org:8080/#/c/21437/16/fe/src/main/java/org/apache/impala/catalog/HdfsTableLoadParamsBuilder.java@77 PS16, Line 77: setFileMetadata nit: use a better name, e.g. setLoadPartitionFileMetadata? 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: Set of all partition names targeted > Ack Not done yet. Can we use a better method name? 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: new HashSet<> Why need a new HashSet instance here? BTW, this is not a good usage of the builder pattern. We add it to make the code more clear. But now we still use the long constructor which makes no difference with the original code. This might be better: ((HdfsTable) table).load( new HdfsTableLoadParamsBuilder(msClient.getHiveClient(), msTbl) .setIsReuseMetadata(true) .setLoadPartitionFileMetadata(false) .setLoadTableSchema(true) .setRefreshUpdatedPartitions(true) .setPartitionsToUpdate(partsToCreate) .setDebugAction(update.getDebug_action()) .setReason("Preload for INSERT") .setIsPreLoadForInsert(true) .setCatalogTimeline(catalogTimeline) .build()); -- 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: 16 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, 28 Jun 2024 02:38:30 +0000 Gerrit-HasComments: Yes
