[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/20506 )
Change subject: IMPALA-12465: Unicode column name support ...................................................................... Patch Set 11: (18 comments) > Uploaded patch set 11: Commit message was updated. 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 validation and uses > Line width in commit messages is at most 72, here they are often broken unn Done http://gerrit.cloudera.org:8080/#/c/20506/10//COMMIT_MSG@18 PS10, Line 18: > Hive Done 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 databas Yes, we need it for the 'SHOW tables' queries. It would also be better to keep it if we add soe more tests in future and need a database variable. 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, on I was just trying to make sure that this query works for a table with unicode column names. This just helps with covering all possible testcases. 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? We can remove them. Removed in the latest patch. 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 Yeah, I agree that it'll get cleaned automatically. The query is just to cover all testcases and to check whether the drop statements are working fine. 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. Yeah, I agree that it'll get cleaned automatically. The query is just to cover all testcases and to check whether the drop statements are working fine. 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? We can remove them. Removed in the latest patch. 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. Yeah, I agree that it'll get cleaned automatically. The query is just to cover all testcases and to check whether the drop statements are working fine. 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? We can remove them. Removed in the latest patch. 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. Yeah, I agree that it'll get cleaned automatically. The query is just to cover all testcases and to check whether the drop statements are working fine. 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? We can remove them. Removed in the latest patch. 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. Yeah, I agree that it'll get cleaned automatically. The query is just to cover all testcases and to check whether the drop statements are working fine. 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. Yeah, I agree that it'll get cleaned automatically. The query is just to cover all testcases and to check whether the drop statements are working fine. 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? We can remove them. Removed in the latest patch. 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. Yeah, I agree that it'll get cleaned automatically. The query is just to cover all testcases and to check whether the drop statements are working fine. http://gerrit.cloudera.org:8080/#/c/20506/5/tests/metadata/column-unicode.py File tests/metadata/column-unicode.py: http://gerrit.cloudera.org:8080/#/c/20506/5/tests/metadata/column-unicode.py@12 PS5, Line 12: > Are you planning to add the constraint back? Should we? I feel this way if anyone runs the file, they can see the different working formats which might also help QE further. 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 " I think it'd be better to keep them separate if we plan to add more testcases. -- 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: 11 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 16:23:13 +0000 Gerrit-HasComments: Yes
