wangsheng 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 3:
(6 comments)
Thanks for review, Zoltan. I found that even if insert into 'NOT NULL'
column(required) a null value, the result is success. If we insert the new test
table in iceberg-create.test like this:
insert into iceberg_nullable_test (null,cast('2021-01-01' as timestamp),null);
The result is success, and we can alse select this table and get this record. I
think this is innormal, we cannot insert a null value to required column.
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:
> nit: Commit message lines should be under 72 chars
Done
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 | nullable |
: +------+--------+---------+----------+
: | id | int | | false |
: | name | string | | true |
: | age | int | | true |
: +------+--------+---------+----------+
: And 'SHOW CREATE TABLE XXX' will also display 'NULL'/'NOT NULL'
: proper
> Could you please add tests for these?
Done
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 p
Done
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
Done
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() a
Done
I found that we don't need these methods anymore, so I removed related code.
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: nullable
> nit: maybe use Impala/SQL terminology here, i.e. "nullable", just like for
Done
--
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: 3
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: Tue, 05 Jan 2021 07:36:28 +0000
Gerrit-HasComments: Yes
