[email protected] has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20506 )

Change subject: IMPALA-12465: Unicode column name support
......................................................................


Patch Set 23:

(10 comments)

> Uploaded patch set 23.

Sorry for the delay, I was on leave today.

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: lysisError("alter t
> It should be "Valid unicode column names."
Done


http://gerrit.cloudera.org:8080/#/c/20506/19/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@466
PS19, Line 466:         "Column name conflicts with existing partition column: 
year");
> Move these up to the other AnalyzesOk calls, after L449.
Done


http://gerrit.cloudera.org:8080/#/c/20506/19/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@467
PS19, Line 467:
> In TestAlterTableAddColumn() and TestAlterTableAddColumns() I don't think w
Done


http://gerrit.cloudera.org:8080/#/c/20506/19/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@532
PS19, Line 532:     AnalysisError("alter table functional.alltypes add if not 
exists columns (year int)",
> It should be "Valid unicode column names."
Done


http://gerrit.cloudera.org:8080/#/c/20506/19/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@533
PS19, Line 533:         "Column name conflicts with existing partition column: 
year");
> Move these up to the other AnalyzesOk calls, after L449.
Done


http://gerrit.cloudera.org:8080/#/c/20506/19/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@535
PS19, Line 535:     // Duplicate column names.
> line too long (94 > 90)
Done


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.
Done


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).
Done


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.
Done


http://gerrit.cloudera.org:8080/#/c/20506/22/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/22/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@710
PS22, Line 710:         "Column already exists: tinyint_col");
> nit: move up to line 699
Sure, I can move it in this function but in a lot of other functions its all 
mixed up, so I think we should leave it as it is in those.



--
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: 23
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: Thu, 14 Dec 2023 19:00:00 +0000
Gerrit-HasComments: Yes

Reply via email to