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 <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 28 Mar 2018 00:13:22 +0000 Gerrit-HasComments: Yes
