Alex Behm has posted comments on this change. Change subject: IMPALA-2636: HS2 GetTables() returns TABLE_TYPE as TABLE for VIEW ......................................................................
Patch Set 2: (12 comments) 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"; > Hive does return VIRTUAL_VIEW if "hive.server2.table.type.mapping" is set t Agree. Only two types of tables for us: TABLE or VIEW. Let's avoid doing something like "hive.server2.table.type.mapping" which sounds like unnecessary complexity to me. Line 298: if (table.getMetaStoreTable() != null) { > I ran this locally and sometime I get null, not sure why! Looks like you have a solution, but let's spend the time to understand the problem. I can help. http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java File fe/src/main/java/org/apache/impala/service/MetadataOp.java: Line 468 Let's keep this hasValidTableType type flag. Calling getDbsMetadata() can be expensive and we don't want to do it unnecessarily. Line 63: private static final String TABLE_TYPE_INDEX_TABLE = "INDEX"; TABLE_TYPE_INDEX Line 77: // GetTableTypes returns values: "TABLE", "VIEW", "INDEX" Remove "INDEX", see comment below Line 494: List<String> upperCaseTypes = null; upperCaseTableTypes Line 495: if (tableTypes != null && !tableTypes.isEmpty()) { Let's add tests to cover passing in TABLE, VIEW or both. Line 514: remove blank line (the following check still belongs to tableType) Line 515: if (upperCaseTypes != null && !upperCaseTypes.contains(tableType.toUpperCase())) continue; No need tor tableType.toUpperCase() - we know if must already be upper case (otherwise we must have a bug somewhere) Line 635: * Fills the GET_TYPEINFO_RESULTS with "TABLE", "VIEW", "INDEX". I'm thinking we should remove "INDEX" here so as not to confuse clients and users. Impala cannot load index tables so we should not advertise them as a valid table type (you will always get an empty result set if you ask for index tables). http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/test/java/org/apache/impala/service/FrontendTest.java File fe/src/test/java/org/apache/impala/service/FrontendTest.java: Line 308: assertEquals("INDEX", resp.rows.get(2).getColVals().get(0).string_val); Remove, I don't think we should advertise this as a valid table type - Impala cannot load or use Hive indexes. http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/test/java/org/apache/impala/service/JdbcTest.java File fe/src/test/java/org/apache/impala/service/JdbcTest.java: Line 200: assertEquals(rs.getString(1).toLowerCase(), "index"); remove -- 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: 2 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-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Reviewer: sandeep akinapelli <[email protected]> Gerrit-HasComments: Yes
