Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10912 )
Change subject: IMPALA-7257. Support Kudu tables in LocalCatalog ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java File fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java: http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java@369 PS3, Line 369: for (KuduPartitionParam partitionParam: table.getPartitionBy()) { > Why putting them here instead of KuduTable? I've been somewhat inconsistent with what goes into KuduTable or HdfsTable vs FeCatalogUtils. I'll move this there. http://gerrit.cloudera.org:8080/#/c/10912/3/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/10912/3/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@258 PS3, Line 258: public static List<KuduPartitionParam> loadPartitionByParams( > What's the future plan for those static methods shared with local table? I think eventually they could become default methods in the FeKuduTable interface. However right now I wanted to preserve the ability to backport this back to the 2.x branch so I've been avoiding Java8-only features and making static methods either here or in FeCatalogUtils. I was inconsistent in this patch, so I'll move that FeCatalogUtils static method back here. http://gerrit.cloudera.org:8080/#/c/10912/3/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/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@140 PS3, Line 140: kuduTableName_ instead of name below > Why can't we do the same here? I think that was a bug in the old implementation. The name that shows up in the descriptor should be the name of the table as seen by Impala, rather than the name as seen by Kudu, don't you think? I think the only issue it would cause is potentially wrong messages in the plan or summary output. http://gerrit.cloudera.org:8080/#/c/10912/3/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java@147 PS3, Line 147: // TODO(todd) > please explain oops, this was a bug. It was a TODO to setKey_columns, but then I did the TODO and forgot to remove this stub. Interesting that the code still works, though, despite not setting the key columns in the descriptor. -- To view, visit http://gerrit.cloudera.org:8080/10912 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b6a317ee895e43e00ade953e814867b56b4e6dd Gerrit-Change-Number: 10912 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Thu, 19 Jul 2018 02:07:36 +0000 Gerrit-HasComments: Yes
