Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20506 )
Change subject: IMPALA-12465: Unicode column name support ...................................................................... Patch Set 19: (8 comments) Thanks. We could vary some of the column names a bit, see comments. http://gerrit.cloudera.org:8080/#/c/20506/19/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/20506/19/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@465 PS19, Line 465: Valid column names. It should be "Valid unicode column names." http://gerrit.cloudera.org:8080/#/c/20506/19/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@466 PS19, Line 466: AnalyzesOk("alter table functional.alltypes add column `???` int"); Move these up to the other AnalyzesOk calls, after L449. http://gerrit.cloudera.org:8080/#/c/20506/19/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@467 PS19, Line 467: AnalyzesOk("alter table functional.alltypes add column `EDW최종변경일시` int"); In TestAlterTableAddColumn() and TestAlterTableAddColumns() I don't think we need more than one unicode case per test. For example here we could keep '???' and in TestAlterTableAddColumns() around L532 we could have a column name that mixes Korean letters, a question mark and Devanagari. http://gerrit.cloudera.org:8080/#/c/20506/19/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@532 PS19, Line 532: // Valid column names. It should be "Valid unicode column names." http://gerrit.cloudera.org:8080/#/c/20506/19/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@533 PS19, Line 533: AnalyzesOk("alter table functional.alltypes add columns (`시스템처리최종사용자번호` int)"); > line too long (107 > 90) Move these up to the other AnalyzesOk calls, after L449. http://gerrit.cloudera.org:8080/#/c/20506/19/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@604 PS19, Line 604: (`???` We could add some Devanagari. http://gerrit.cloudera.org:8080/#/c/20506/19/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@712 PS19, Line 712: `???` We could add some Chinese char(s). http://gerrit.cloudera.org:8080/#/c/20506/19/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1481 PS19, Line 1481: `???` We could add some Korean letters. -- 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: 19 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, 13 Dec 2023 12:04:49 +0000 Gerrit-HasComments: Yes
