[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

["Alex Behm (Code Review)" <ge...@cloudera.org>]

Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8851 )

Change subject: IMPALA-3193: Show table's comment on show tables
......................................................................


Patch Set 19:

(3 comments)

Nice work!

Thanks for your patience with reviews. Dimitris is on vacation this week,  and this change somehow slipped my radar. Thanks Jim for bringing this to my attention.

This patch is ready to merge after my minor remaining comments.

http://gerrit.cloudera.org:8080/#/c/8851/19/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8851/19/be/src/service/client-request-state.cc@269
PS19, Line 269:       DCHECK(names.size() == comments.size());
no need for this, already checked in SetResultSet()


http://gerrit.cloudera.org:8080/#/c/8851/19/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
File fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java:

http://gerrit.cloudera.org:8080/#/c/8851/19/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java@128
PS19, Line 128:   public Table getTable(String dbName, String tableName) {
I think we should remove these overrides because they can be super hard to reason about.

If there are FE unit tests that need tables loaded before running, then I think those tests should do what is done in getTables() here.

For an example where we follow that preferred practice, take a look at FrontendTest.TestGetTablesTypeTable()


http://gerrit.cloudera.org:8080/#/c/8851/19/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java@136
PS19, Line 136:   public List<Table> getTables(String dbName, PatternMatcher matcher)
Please remove if possible, see above.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 19
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Comment-Date: Sat, 10 Mar 2018 00:03:43 +0000
Gerrit-HasComments: Yes