[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/20506 )
Change subject: IMPALA-12465: Unicode column name support ...................................................................... Patch Set 6: (21 comments) > Uploaded patch set 6. http://gerrit.cloudera.org:8080/#/c/20506/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20506/3//COMMIT_MSG@9 PS3, Line 9: Hive > Nit: capitalise Hive. Done http://gerrit.cloudera.org:8080/#/c/20506/3//COMMIT_MSG@11 PS3, Line 11: Hive > Nit: capitalise Hive. Done http://gerrit.cloudera.org:8080/#/c/20506/3//COMMIT_MSG@15 PS3, Line 15: places > 'Allows' sounds strange here, with respect to restrictions, it could be som Done http://gerrit.cloudera.org:8080/#/c/20506/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20506/5//COMMIT_MSG@17 PS5, Line 17: > This sentence can be removed as the patch adds testing. Normally I would pr Noted, I've added a testing header. http://gerrit.cloudera.org:8080/#/c/20506/5/testdata/datasets/functional/functional_schema_template.sql File testdata/datasets/functional/functional_schema_template.sql: http://gerrit.cloudera.org:8080/#/c/20506/5/testdata/datasets/functional/functional_schema_template.sql@4312 PS5, Line 4312: > This table seems to be unused. Ack, removed. http://gerrit.cloudera.org:8080/#/c/20506/5/testdata/workloads/functional-query/queries/QueryTest/create-table-unicode.test File testdata/workloads/functional-query/queries/QueryTest/create-table-unicode.test: http://gerrit.cloudera.org:8080/#/c/20506/5/testdata/workloads/functional-query/queries/QueryTest/create-table-unicode.test@3 PS5, Line 3: > Do these names cover multi-byte characters? I'd suggest have a column name Done http://gerrit.cloudera.org:8080/#/c/20506/5/testdata/workloads/functional-query/queries/QueryTest/create-table-unicode.test@50 PS5, Line 50: > I think test dimensions across different tables can take care of testing al Yeah I agree that there's no difference in create statements, but the output for many queries like describe differs for different file formats, that's why we need to handle each file format separately. http://gerrit.cloudera.org:8080/#/c/20506/5/testdata/workloads/functional-query/queries/QueryTest/create-table.test File testdata/workloads/functional-query/queries/QueryTest/create-table.test: http://gerrit.cloudera.org:8080/#/c/20506/5/testdata/workloads/functional-query/queries/QueryTest/create-table.test@362 PS5, Line 362: ==== > I'd prefer not to modify this file if there's no substantial change. You ca Done 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@3 PS5, Line 3: > flake8 is a Python linter, it's highlighting issues that'll need to be fixe Done http://gerrit.cloudera.org:8080/#/c/20506/5/tests/metadata/column-unicode.py@6 PS5, Line 6: > For test naming, file and test class should be similar and match others in Done http://gerrit.cloudera.org:8080/#/c/20506/5/tests/metadata/column-unicode.py@12 PS5, Line 12: > If these are quick tests that apply to all table formats, I would omit over I've removed the constraints, we'll need some dimensions to run the tests for different impala client protocols. http://gerrit.cloudera.org:8080/#/c/20506/5/tests/metadata/column-unicode.py@19 PS5, Line 19: > flake8: E501 line too long (102 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/20506/5/tests/metadata/column-unicode.py@19 PS5, Line 19: > flake8: W504 line break after binary operator Done http://gerrit.cloudera.org:8080/#/c/20506/5/tests/metadata/column-unicode.py@20 PS5, Line 20: > flake8: E501 line too long (102 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/20506/5/tests/metadata/column-unicode.py@21 PS5, Line 21: > flake8: E501 line too long (107 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/20506/5/tests/metadata/column-unicode.py@21 PS5, Line 21: > flake8: E116 unexpected indentation (comment) Done http://gerrit.cloudera.org:8080/#/c/20506/5/tests/metadata/column-unicode.py@24 PS5, Line 24: > flake8: W292 no newline at end of file Done http://gerrit.cloudera.org:8080/#/c/20506/5/tests/metadata/column-unicode.py@24 PS5, Line 24: > flake8: E501 line too long (92 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/20506/5/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/20506/5/tests/shell/test_shell_interactive.py@389 PS5, Line 389: > flake8: E501 line too long (99 > 90 characters) Done http://gerrit.cloudera.org:8080/#/c/20506/5/tests/shell/test_shell_interactive.py@389 PS5, Line 389: # IMPALA-10415: Multi queries in a line containing unicode chars. > This feels like a different test. Please split it into a new function named Done http://gerrit.cloudera.org:8080/#/c/20506/5/tests/shell/test_shell_interactive.py@404 PS5, Line 404: > Can we also validate the output column headers are formatted correctly? Sho Done -- 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: 6 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: Mon, 04 Dec 2023 07:55:29 +0000 Gerrit-HasComments: Yes
