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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
