Zoltan Borok-Nagy 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: (7 comments) Thanks for the comments! 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, b This is only the write side. But now that I'm setting the file format for the prototype partition in IcebergCtasTarget this line is no longer needed. 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@502 PS1, Line 502: IcebergPartitionSpec partSpec = ((FeIcebergTable)table_).getDefaultPartitionSpec(); > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/17130/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@504 PS1, Line 504: throw new AnalysisException("PARTITION clause is only valid for INSERT into " + > line too long (91 > 90) Done 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. Done 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 Done 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 ge Thanks, removed them from this class. 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 DC Done -- 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: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: wangsheng <[email protected]> Gerrit-Comment-Date: Tue, 09 Mar 2021 18:27:53 +0000 Gerrit-HasComments: Yes
