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 <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: wangsheng <[email protected]> Gerrit-Comment-Date: Tue, 02 Mar 2021 15:19:43 +0000 Gerrit-HasComments: Yes
