Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16904 )
Change subject: IMPALA-10368: Support required/optional property when creating Iceberg table ...................................................................... Patch Set 2: (6 comments) Thanks for working on this, the change LGTM. Only found some nits, and please add some tests. http://gerrit.cloudera.org:8080/#/c/16904/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16904/2//COMMIT_MSG@9 PS2, Line 9: tch. nit: Commit message lines should be under 72 chars http://gerrit.cloudera.org:8080/#/c/16904/2//COMMIT_MSG@13 PS2, Line 13: Besides, 'DESCRIBE XXX' for Iceberg table will display 'optional' property like : this: : +------+--------+---------+----------+ : | name | type | comment | optional | : +------+--------+---------+----------+ : | id | int | | false | : | name | string | | true | : | age | int | | true | : +------+--------+---------+----------+ : And 'SHOW CREATE TABLE XXX' will also display 'NULL'/'NOT NULL' property for Iceberg : table. Could you please add tests for these? Also please add tests for INSERTing nulls in optional/required columns. What kind of error message do we get in the latter case? http://gerrit.cloudera.org:8080/#/c/16904/2/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/16904/2/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@275 PS2, Line 275: Column nit: this method analyzes all the columns, so the method name could be in plural form, i.e. 'analyzeIcebergColumns' http://gerrit.cloudera.org:8080/#/c/16904/2/fe/src/main/java/org/apache/impala/analysis/TableDef.java File fe/src/main/java/org/apache/impala/analysis/TableDef.java: http://gerrit.cloudera.org:8080/#/c/16904/2/fe/src/main/java/org/apache/impala/analysis/TableDef.java@434 PS2, Line 434: hasKuduOptions() nit: maybe in the long-term (in a separate jira/commit) it would be nice to have IcebergOptions, so we wouldn't hijack KuduOptions. http://gerrit.cloudera.org:8080/#/c/16904/2/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java: http://gerrit.cloudera.org:8080/#/c/16904/2/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@a357 PS2, Line 357: : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : We have similar logic in IcebergSchemaConverter's convertToImpalaSchema() and toImpalaType(). Could you refactor the code so we'd only have this logic at one place, in IcebergSchemaConverter? http://gerrit.cloudera.org:8080/#/c/16904/2/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/16904/2/fe/src/main/java/org/apache/impala/service/Frontend.java@489 PS2, Line 489: optional nit: maybe use Impala/SQL terminology here, i.e. "nullable", just like for Kudu. -- To view, visit http://gerrit.cloudera.org:8080/16904 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I70b8014ba99f43df1b05149ff7a15cf06b6cd8d3 Gerrit-Change-Number: 16904 Gerrit-PatchSet: 2 Gerrit-Owner: wangsheng <[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: Mon, 04 Jan 2021 15:09:34 +0000 Gerrit-HasComments: Yes
