Daniel Becker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20506 )

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


Patch Set 11:

(4 comments)

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
> Yes, we need it for the 'SHOW tables' queries. It would also be better to k
Actually you don't need it in SHOW TABLES either, you can just write
  show tables;
This will print the tables in the current database.

If you add more tests where it is needed we can add $DATABASE for those tests, 
we shouldn't have them for tests where it is not needed. Also, I don't think it 
will be needed for more tests, we're not planning to do cross-database testing.


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 was just trying to make sure that this query works for a table with unico
Ok, we can leave this test in.


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;
> Yeah, I agree that it'll get cleaned automatically. The query is just to co
Ok.


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
> I think it'd be better to keep them separate if we plan to add more testcas
Ok, you can leave it like that. But in that case extracting the table name into 
a variable could also make sense in case you later want to change the name - it 
could lead to a bug if we forget to change the name 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: 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:40:39 +0000
Gerrit-HasComments: Yes

Reply via email to