Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/19114 )

Change subject: IMPALA-11607: Support for TPC-H and TPC-DS test datasets stored 
as Iceberg tables
......................................................................


Patch Set 10:

(2 comments)

Some small enhancement requests, otherwise LGTM!

http://gerrit.cloudera.org:8080/#/c/19114/10/testdata/bin/generate-schema-statements.py
File testdata/bin/generate-schema-statements.py:

http://gerrit.cloudera.org:8080/#/c/19114/10/testdata/bin/generate-schema-statements.py@351
PS10, Line 351: col.startswith
maybe use col.lower().startswith(("primary key", "foreign key"))

That will also work for "PrImAry kEy" and "fOreIGN Key"

And maybe we should use strip() as well.


http://gerrit.cloudera.org:8080/#/c/19114/10/testdata/bin/generate-schema-statements.py@353
PS10, Line 353:       # Omit PRIMARY KEY declaration e.g 'col_i INT PRIMARY 
KEY,'
              :       col = col.replace(" primary key", "").replace(" PRIMARY 
KEY", "")
              :       # Iceberg tables do not support TINYINT and SMALLINT
              :       col = col.replace(" tinyint", " int").replace(" TINYINT", 
" INT")
              :       col = col.replace(" smallint", " int").replace(" 
SMALLINT", " INT")
Maybe use case-insensitive regex replace here as well.



--
To view, visit http://gerrit.cloudera.org:8080/19114
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8bbc5209e7e649f67a48144a2844b35d9f9c7f1
Gerrit-Change-Number: 19114
Gerrit-PatchSet: 10
Gerrit-Owner: Anonymous Coward <lipeng...@sensorsdata.cn>
Gerrit-Reviewer: Anonymous Coward <lipeng...@sensorsdata.cn>
Gerrit-Reviewer: Gergely Fürnstáhl <gfurnst...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tma...@apache.org>
Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com>
Gerrit-Comment-Date: Fri, 21 Oct 2022 14:07:49 +0000
Gerrit-HasComments: Yes

Reply via email to