Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16120 )
Change subject: IMPALA-9903: Reduce Kudu openTable calls per query ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/16120/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16120/2//COMMIT_MSG@9 PS2, Line 9: This patch reduces the number of Kudu openTable calls for the : lifetime of a query by storing the KuduTable object in the : Analyzer GlobalState and using it in the > I think it would be good to be more specific here. Looks like currently we Done http://gerrit.cloudera.org:8080/#/c/16120/2/fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java File fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java: http://gerrit.cloudera.org:8080/#/c/16120/2/fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java@166 PS2, Line 166: result.setSchema(resultSchema); > These are methods that implement the show partitions DDL, so we don't need Done http://gerrit.cloudera.org:8080/#/c/16120/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: http://gerrit.cloudera.org:8080/#/c/16120/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@185 PS2, Line 185: @Override : public List<String> getPrimaryKeyColumnNames() { : return ImmutableList.copyOf(primaryKeyColumnNames_); : } : > This would mean that once kuduTable_ is initialized, it never gets refreshe Done http://gerrit.cloudera.org:8080/#/c/16120/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@298 PS2, Line 298: partitionBy_ = Utils.loadPartitionByParams(kuduTable); > This probably should be kept as is otherwise we won't see a updated Kudu sc Done http://gerrit.cloudera.org:8080/#/c/16120/2/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java: http://gerrit.cloudera.org:8080/#/c/16120/2/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@56 PS2, Line 56: /** > Caching it in LocalTable makes sense since it's per-query anyway. So this p If we are going the analyzer route I don't think this is needed right? http://gerrit.cloudera.org:8080/#/c/16120/2/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java File fe/src/main/java/org/apache/impala/planner/KuduScanNode.java: http://gerrit.cloudera.org:8080/#/c/16120/2/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java@135 PS2, Line 135: // Get the KuduTable from the analyzer to retrieve the cached KuduTable > I think this invocation should go via 'analyzer' to retrieve the per-query Done -- To view, visit http://gerrit.cloudera.org:8080/16120 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec12a5be9b30e19a123142af5453a91bd4300b63 Gerrit-Change-Number: 16120 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Wed, 22 Jul 2020 18:39:14 +0000 Gerrit-HasComments: Yes
