Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9090 )

Change subject: IMPALA-5903: Inconsistent specification of result set and 
result set metadata
......................................................................


Patch Set 4:

(4 comments)

This change makes a lot of sense to me. My main concern is getting this as 
close to right as possible the first ime, because the messages will be 
user-visible and if we change them in future there's some risk of breaking 
scripts that depend on the existing text (not that people *should* be depending 
on this for the most part, but they probably will).

http://gerrit.cloudera.org:8080/#/c/9090/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9090/4//COMMIT_MSG@14
PS4, Line 14: CalatopOpExecutor
typo


http://gerrit.cloudera.org:8080/#/c/9090/4/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:

http://gerrit.cloudera.org:8080/#/c/9090/4/fe/src/main/java/org/apache/impala/service/Frontend.java@503
PS4, Line 503:     if (ddl.op_type == TCatalogOpType.DDL) {
This might need a one-line comment, e.g. "All DDL commands return a string 
summarizing the outcome of the DDL."


http://gerrit.cloudera.org:8080/#/c/9090/4/fe/src/main/java/org/apache/impala/service/Frontend.java@504
PS4, Line 504:       metadata.setColumns(Arrays.asList(new TColumn("summary", 
Type.STRING.toThrift())));
Can we add a corresponding assert (probably in impala-server.cc) that all 
(successful?) DDL operations have a result set set?


http://gerrit.cloudera.org:8080/#/c/9090/4/testdata/workloads/functional-query/queries/QueryTest/alter-table.test
File testdata/workloads/functional-query/queries/QueryTest/alter-table.test:

http://gerrit.cloudera.org:8080/#/c/9090/4/testdata/workloads/functional-query/queries/QueryTest/alter-table.test@234
PS4, Line 234: jointbl_test
it's a bit inconsistent that we include the table name in this message but not 
others. Might be simplest to not include the table name, since it doesn't add 
information you can't infer from the original statement and avoids other 
questions, e.g. "should we quote the table name in backticks?"



--
To view, visit http://gerrit.cloudera.org:8080/9090
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic542fb8e49e850052416ac663ee329ee3974e3b9
Gerrit-Change-Number: 9090
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com>
Gerrit-Comment-Date: Wed, 28 Mar 2018 00:13:22 +0000
Gerrit-HasComments: Yes

Reply via email to