Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14422 )
Change subject: IMPALA-8738: Extend "show tables" to return values other than the table name ...................................................................... Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/14422/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14422/4//COMMIT_MSG@17 PS4, Line 17: Testing: Could you also add a test case to: - ParserTest.TestShow() - testdata/workloads/functional-query/queries/QueryTest/show.test, which is run by tests/metadata/test_metadata_query_statements.py::test_show http://gerrit.cloudera.org:8080/#/c/14422/4/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/14422/4/fe/src/main/cup/sql-parser.cup@2465 PS4, Line 2465: {: RESULT = new ShowTablesStmt(false); :} Did my suggestion to add an 'opt_extended' rule not work for some reason? http://gerrit.cloudera.org:8080/#/c/14422/4/fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java File fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java: http://gerrit.cloudera.org:8080/#/c/14422/4/fe/src/main/java/org/apache/impala/analysis/ShowTablesStmt.java@53 PS4, Line 53: true ,the true, the http://gerrit.cloudera.org:8080/#/c/14422/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/14422/4/fe/src/main/java/org/apache/impala/service/Frontend.java@815 PS4, Line 815: new ArrayList<>(); Lists.newArrayList() http://gerrit.cloudera.org:8080/#/c/14422/4/fe/src/main/java/org/apache/impala/service/Frontend.java@817 PS4, Line 817: new HashSet<>(); Sets.newHashSet() http://gerrit.cloudera.org:8080/#/c/14422/4/fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java: http://gerrit.cloudera.org:8080/#/c/14422/4/fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java@234 PS4, Line 234: List<String> tables = The formatting in this function is pretty ugly now. Not the biggest deal, but you could improve it, for example by doing something like: PatternMatcher pattern1 = PatternMatcher.createHivePatternMatcher("*"); and then .getTableNames("functional", pattern1, USER, false) http://gerrit.cloudera.org:8080/#/c/14422/4/fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java@275 PS4, Line 275: public void TestShowExtendedTableResultsFiltered() throws ImpalaException { Since "EXTENDED" doesn't really change anything about the authorization of the query or the pattern matching, I think its probably not necessary to check all of these cases with "extended=true", just a single one would be fine. So, instead of adding this additional test, could you just take a single case from here, doesn't matter which one as long as its one that actually does return some table types, and put it in the test above. For example, by taking lines 279-282 and moving them up to line 241, which a comment says "Test for SHOW EXTENDED" or similar. -- To view, visit http://gerrit.cloudera.org:8080/14422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I63057c9d2fc453f95c6890bdc90e11c61a98a419 Gerrit-Change-Number: 14422 Gerrit-PatchSet: 4 Gerrit-Owner: Anonymous Coward <cnsdjnfan...@qq.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Mon, 21 Oct 2019 19:18:50 +0000 Gerrit-HasComments: Yes
