[Impala-ASF-CR] IMPALA-2636: HS2 GetTables() returns TABLE TYPE as TABLE for VIEW

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

Alex Behm has posted comments on this change.

Change subject: IMPALA-2636: HS2 GetTables() returns TABLE_TYPE as TABLE for VIEW
......................................................................


Patch Set 1:

(10 comments)

Nice, thanks for working on this one

http://gerrit.cloudera.org:8080/#/c/7353/1/fe/src/main/java/org/apache/impala/service/MetadataOp.java
File fe/src/main/java/org/apache/impala/service/MetadataOp.java:

Line 62:   private static final String TABLE_TYPE_VIEW = "VIEW";
Please check if this is consistent with what Hive returns. Does Hive return "VIRTUAL_VIEW"?


Line 63:   private static final String TABLE_TYPE_INDEX_TABLE = "INDEX_TABLE";
Why not just "INDEX"?
(Not that it really matters too much since Impala cannot load these anyway)


Line 298:             if (table.getMetaStoreTable() != null) {
Why this null check? Loaded tables should really have this set.


Line 321:     if(typeStr == null) {
single line and space after "if"


Line 324:     typeStr = typeStr.toUpperCase();
Is this really needed? If so, please Inline into L326.


Line 328:       case EXTERNAL_TABLE:
indent case by 2


Line 338:     } catch (Exception e) {
move this up so it's clear the switch block cannot throw


Line 495:     // Impala catalog only contains TABLE. Returns an empty set if the search does not
This should be fixed as well.


http://gerrit.cloudera.org:8080/#/c/7353/1/fe/src/test/java/org/apache/impala/service/FrontendTest.java
File fe/src/test/java/org/apache/impala/service/FrontendTest.java:

Line 221:   public void TestGetView() throws ImpalaException {
We should test two cases:
1. View that is not loaded is shown as a table
2. View that is loaded is shown as a view

For testing the first case you should be able to use addTestDb() and addTestView()

For testing the second case, add a dummy AnalyzesOk("select * from functional.alltypes_view") to make sure the view is really loaded at this point - otherwise the test relies on the view already being loaded and might be flaky


Line 226:     req.get_tables_req.setSchemaName("functional");
No need to test tables here


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I90616388e6181cf342b3de389af940214ed46428
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes