Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/16446 )
Change subject: IMPALA-10164: Supporting HadoopCatalog for Iceberg table ...................................................................... Patch Set 1: (8 comments) Thanks for taking care of this patch! I have some comments but nothing serious as the patch is in good shape in my opinion. What I miss is some test coverage to cover at least the new syntax. http://gerrit.cloudera.org:8080/#/c/16446/1/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/16446/1/common/thrift/CatalogObjects.thrift@96 PS1, Line 96: identitied nit: typo http://gerrit.cloudera.org:8080/#/c/16446/1/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java File fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java: http://gerrit.cloudera.org:8080/#/c/16446/1/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@244 PS1, Line 244: getIcebergCatalog I find it confusing that IcebergUtil also has a function with the same name but different parameterisation. Could you consider moving this one to that class next to that function with the same name? http://gerrit.cloudera.org:8080/#/c/16446/1/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergTable.java: http://gerrit.cloudera.org:8080/#/c/16446/1/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@71 PS1, Line 71: public static final String ICEBERG_CATALOG = "iceberg.catalog"; nit: pls add comment similarly to ICEBERG_FILE_FORMAT above. http://gerrit.cloudera.org:8080/#/c/16446/1/fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java: http://gerrit.cloudera.org:8080/#/c/16446/1/fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java@111 PS1, Line 111: @Override : public TIcebergCatalog getIcebergCatalog() { : return icebergCatalog_; : } FeIcebergTable has 2 implementations, this class and IcebergTable. Both have the same implementation of getIcebergCatalog(). Please consider move the implementation of this function into FeIcebergTable. http://gerrit.cloudera.org:8080/#/c/16446/1/fe/src/main/java/org/apache/impala/catalog/local/LocalIcebergTable.java@183 PS1, Line 183: getIcebergCatalog If getIcebergCatalog(msTable) was moved to IcebergUtil then we could use it here. http://gerrit.cloudera.org:8080/#/c/16446/1/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java File fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java: http://gerrit.cloudera.org:8080/#/c/16446/1/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java@a93 PS1, Line 93: As I see this patch also contains some modifications related to Iceberg table location. Could you a) move it to a separate patch b) mention these changes in the commit message? http://gerrit.cloudera.org:8080/#/c/16446/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/16446/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@54 PS1, Line 54: String catalog Using TIcebergCatalog as a param would be easier to understand what is the intention of this param. http://gerrit.cloudera.org:8080/#/c/16446/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@63 PS1, Line 63: } else { Would worth a Preconditions check in the else branch that icebergCatalog == HADOOP_TABLES -- To view, visit http://gerrit.cloudera.org:8080/16446 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic1893c50a633ca22d4bca6726c9937b026f5d5ef Gerrit-Change-Number: 16446 Gerrit-PatchSet: 1 Gerrit-Owner: wangsheng <sky...@163.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Mon, 14 Sep 2020 16:16:03 +0000 Gerrit-HasComments: Yes
