You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org> on 2020/06/12 01:32:46 UTC
[Impala-ASF-CR] IMPALA-9688: Support create iceberg table by impala
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 <sk...@163.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Fri, 12 Jun 2020 01:32:46 +0000
Gerrit-HasComments: Yes