Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15797 )
Change subject: IMPALA-9688: Support create iceberg table by impala ...................................................................... Patch Set 21: (11 comments) Thanks for contributing this patch. I think this would be a great addition. I took a look and I mostly have some minor questions/comments. Other than that the patch looks good to me. http://gerrit.cloudera.org:8080/#/c/15797/21//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15797/21//COMMIT_MSG@11 PS21, Line 11: create table Are alter operations on iceberg table supported? Is that coming in subsequent patches? http://gerrit.cloudera.org:8080/#/c/15797/21//COMMIT_MSG@18 PS21, Line 18: partition by spec( : level identity, : event_time identity, : event_time hour, : register_time day : ) Curious about the using a different syntax than 'partitioned by'. Is there a strong reason not to use 'partitioned by' (like other projects using 'partition by spec' for iceberg tables)? Another thing worth pointing out is that the partition spec can include the columns in from the table. This may be good to document either in the code where the table is being created since its a bit different conceptually. http://gerrit.cloudera.org:8080/#/c/15797/21//COMMIT_MSG@28 PS21, Line 28: dispaly nit, spell-check http://gerrit.cloudera.org:8080/#/c/15797/21//COMMIT_MSG@30 PS21, Line 30: parititon nit, spell-check http://gerrit.cloudera.org:8080/#/c/15797/21/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java: http://gerrit.cloudera.org:8080/#/c/15797/21/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@227 PS21, Line 227: nly have one PartitionSpec Do we need a Preconditions check that the size is indeed 0 here? http://gerrit.cloudera.org:8080/#/c/15797/21/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@578 PS21, Line 578: putGeneratedKuduProperty may be rename this method to something more generic now that it is used by both Kudu and Iceberg? http://gerrit.cloudera.org:8080/#/c/15797/21/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/15797/21/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2547 PS21, Line 2547: msClient.getHiveClient().createTable(newTable); One of things which I noticed from the examples in the test is the partition spec can contain the columns from the main table. This probably means that the HMS table is always unpartitioned? If this is correct, may be we should make sure that the partitionKeys in the table before creating the table in HMS here. http://gerrit.cloudera.org:8080/#/c/15797/21/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/15797/21/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@54 PS21, Line 54: icebergDdlLock_ Would be good to document why we need a lock here? Also, given that we are not using this lock anywhere else may be just change this method to synchronized? http://gerrit.cloudera.org:8080/#/c/15797/21/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@86 PS21, Line 86: filed nit, spell-check http://gerrit.cloudera.org:8080/#/c/15797/21/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@169 PS21, Line 169: nextId Is it guaranteed that the threadlocal variable is always set? http://gerrit.cloudera.org:8080/#/c/15797/21/fe/src/main/java/org/apache/impala/util/IcebergUtil.java File fe/src/main/java/org/apache/impala/util/IcebergUtil.java: http://gerrit.cloudera.org:8080/#/c/15797/21/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@51 PS21, Line 51: HadoopTables tables = new HadoopTables(FileSystemUtil.getConfiguration()); : return tables; nit, could be rewritten simply as return new HadoopTables(...) -- To view, visit http://gerrit.cloudera.org:8080/15797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8d85db4c904a8c758c4cfb4f19cfbdab7e6ea284 Gerrit-Change-Number: 15797 Gerrit-PatchSet: 21 Gerrit-Owner: wangsheng <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: wangsheng <[email protected]> Gerrit-Comment-Date: Fri, 12 Jun 2020 01:32:46 +0000 Gerrit-HasComments: Yes
