Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20506 )
Change subject: IMPALA-12465: Unicode column name support ...................................................................... Patch Set 10: (17 comments) Thanks, I think it's ok overall. http://gerrit.cloudera.org:8080/#/c/20506/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20506/10//COMMIT_MSG@9 PS10, Line 9: Impala depends on Hive functions for column name Line width in commit messages is at most 72, here they are often broken unnecessarily, more words would fit on the line. Please go through the commit message and only break the line where necessary / meaningful. http://gerrit.cloudera.org:8080/#/c/20506/10//COMMIT_MSG@18 PS10, Line 18: h Hive http://gerrit.cloudera.org:8080/#/c/20506/10/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test File testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test: http://gerrit.cloudera.org:8080/#/c/20506/10/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test@3 PS10, Line 3: $DATABASE Do we need $DATABASE here? I think the tables will be in the unique database by default. Applies to the other CREATE statements as well. http://gerrit.cloudera.org:8080/#/c/20506/10/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test@8 PS10, Line 8: # Make sure creating a table with the same name doesn't throw an error when I'm not sure we have to test this, this change is not about table names, only column names. Applies to the other similar cases too. http://gerrit.cloudera.org:8080/#/c/20506/10/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test@11 PS10, Line 11: ROW FORMAT DELIMITED : FIELDS TERMINATED BY '\t' : ESCAPED BY '\\' : LINES TERMINATED BY '\n' : STORED AS TEXTFILE; Do we need these? http://gerrit.cloudera.org:8080/#/c/20506/10/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test@45 PS10, Line 45: drop table $DATABASE.testtbl1; We don't need to drop the table because it is created in a unique_database which will be cleaned automatically. http://gerrit.cloudera.org:8080/#/c/20506/10/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test@81 PS10, Line 81: drop table $DATABASE.testtbl_kudu; See L45. http://gerrit.cloudera.org:8080/#/c/20506/10/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test@94 PS10, Line 94: ROW FORMAT DELIMITED : FIELDS TERMINATED BY '\t' : ESCAPED BY '\\' : LINES TERMINATED BY '\n' Do we need these? http://gerrit.cloudera.org:8080/#/c/20506/10/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test@128 PS10, Line 128: drop table $DATABASE.testtbl_iceberg; See L45. http://gerrit.cloudera.org:8080/#/c/20506/10/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test@141 PS10, Line 141: ROW FORMAT DELIMITED : FIELDS TERMINATED BY '\t' : ESCAPED BY '\\' : LINES TERMINATED BY '\n' Do we need these? http://gerrit.cloudera.org:8080/#/c/20506/10/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test@175 PS10, Line 175: drop table $DATABASE.testtbl_orc; See L45. http://gerrit.cloudera.org:8080/#/c/20506/10/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test@188 PS10, Line 188: ROW FORMAT DELIMITED : FIELDS TERMINATED BY '\t' : ESCAPED BY '\\' : LINES TERMINATED BY '\n' Do we need these? http://gerrit.cloudera.org:8080/#/c/20506/10/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test@222 PS10, Line 222: drop table $DATABASE.testtbl_avro; See L45. http://gerrit.cloudera.org:8080/#/c/20506/10/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test@260 PS10, Line 260: drop table $DATABASE.testtbl_part; See L45. http://gerrit.cloudera.org:8080/#/c/20506/10/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test@273 PS10, Line 273: ROW FORMAT DELIMITED : FIELDS TERMINATED BY '\t' : ESCAPED BY '\\' : LINES TERMINATED BY '\n' Do we need this? http://gerrit.cloudera.org:8080/#/c/20506/10/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test@307 PS10, Line 307: drop table $DATABASE.testtbl_parquet; See L45. http://gerrit.cloudera.org:8080/#/c/20506/10/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/20506/10/tests/shell/test_shell_interactive.py@408 PS10, Line 408: test_tbl You could have a 'tbl_name' variable which would include the database and "test_tbl", so you wouldn't need to write "test_tbl" in each query. -- To view, visit http://gerrit.cloudera.org:8080/20506 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1ad9d63ac1b9631a0f4a433798bd5109aa2ed718 Gerrit-Change-Number: 20506 Gerrit-PatchSet: 10 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Wed, 06 Dec 2023 11:42:25 +0000 Gerrit-HasComments: Yes
