You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Gabor Kaszab (Code Review)" <ge...@cloudera.org> on 2021/03/02 15:19:43 UTC
[Impala-ASF-CR] IMPALA-10222: CREATE TABLE AS SELECT for Iceberg tables
Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/17130 )
Change subject: IMPALA-10222: CREATE TABLE AS SELECT for Iceberg tables
......................................................................
Patch Set 1:
(5 comments)
Nice work, Zoltan! I left some comments, nothing too serious.
http://gerrit.cloudera.org:8080/#/c/17130/1/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:
http://gerrit.cloudera.org:8080/#/c/17130/1/be/src/exec/hdfs-table-sink.cc@545
PS1, Line 545: new HdfsParquetTableWriter(
I'm a bit confused here. This is for Iceberg with Parquet as file format, but don't we support other formats such as ORC?
http://gerrit.cloudera.org:8080/#/c/17130/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java:
http://gerrit.cloudera.org:8080/#/c/17130/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@514
PS1, Line 514: if (isStaticPartitioning) {
: throw new AnalysisException("Static partitioning is not supported for " +
: "Iceberg tables.");
: }
You can move this check inside the for loop above.
http://gerrit.cloudera.org:8080/#/c/17130/1/fe/src/main/java/org/apache/impala/catalog/CtasTargetTable.java
File fe/src/main/java/org/apache/impala/catalog/CtasTargetTable.java:
http://gerrit.cloudera.org:8080/#/c/17130/1/fe/src/main/java/org/apache/impala/catalog/CtasTargetTable.java@48
PS1, Line 48: // colsByPos[i] refers to the ith column in the table. The first numClusteringCols are
nit: indentation
http://gerrit.cloudera.org:8080/#/c/17130/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java
File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java:
http://gerrit.cloudera.org:8080/#/c/17130/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java@74
PS1, Line 74: private final FeDb db_;
: private final org.apache.hadoop.hive.metastore.api.Table msTable_;
CtasTargetTble already has members with the same name, abs as I see they get the same value as well. Are these needed here?
http://gerrit.cloudera.org:8080/#/c/17130/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergCtasTarget.java@222
PS1, Line 222: public void addColumn(Column col) {
Can't you use IcebergColumn type for the param? You could get rid of the DCHECK in L223 then.
--
To view, visit http://gerrit.cloudera.org:8080/17130
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I81d2084e401b9fa74d5ad161b51fd3e2aa3fcc67
Gerrit-Change-Number: 17130
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Tue, 02 Mar 2021 15:19:43 +0000
Gerrit-HasComments: Yes