You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org> on 2021/01/04 15:09:34 UTC
[Impala-ASF-CR] IMPALA-10368: Support required/optional property when creating Iceberg table
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 <sk...@163.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Mon, 04 Jan 2021 15:09:34 +0000
Gerrit-HasComments: Yes