sandeep akinapelli has posted comments on this change. ( http://gerrit.cloudera.org:8080/7353 )
Change subject: IMPALA-2636: HS2 GetTables() returns TABLE_TYPE as TABLE for VIEW ...................................................................... Patch Set 3: (12 comments) review comments 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: http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@63 PS2, Line 63: // Result set schema for each of the metadata operations. > TABLE_TYPE_INDEX removed altogether http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@77 PS2, Line 77: > Remove "INDEX", see comment below Done http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@221 PS2, Line 221: > nit: tale -> table Done http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@494 PS2, Line 494: upperCaseTableTypes = Lists.newArrayList(); > upperCaseTableTypes Done http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@495 PS2, Line 495: for (String tableType : tableTypes) { > Let's add tests to cover passing in TABLE, VIEW or both. added tests http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@514 PS2, Line 514: String tableType = dbsMetadata.tableTypes.get(i).get(j); > remove blank line (the following check still belongs to tableType) Done http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@515 PS2, Line 515: if (upperCaseTableTypes != null && !upperCaseTableTypes.contains(tableType)) continue; > No need tor tableType.toUpperCase() - we know if must already be upper case True. removed the unneccesary toupper http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@635 PS2, Line 635: * Fills the GET_TYPEINFO_RESULTS with "TABLE", "VIEW". > I'm thinking we should remove "INDEX" here so as not to confuse clients and Done 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: http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/test/java/org/apache/impala/service/FrontendTest.java@244 PS2, Line 244: > nit: Issue Done http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/test/java/org/apache/impala/service/FrontendTest.java@308 PS2, Line 308: // same as ODBC SQLColumns, which has 18 columns. But the HS2 implementation has > Remove, I don't think we should advertise this as a valid table type - Impa Done 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: http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/test/java/org/apache/impala/service/JdbcTest.java@193 PS2, Line 193: ResultSet rs = con_.getMetaData().getTabl > stale comment? Yep. removed. http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/test/java/org/apache/impala/service/JdbcTest.java@200 PS2, Line 200: } > remove Done -- To view, visit http://gerrit.cloudera.org:8080/7353 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I90616388e6181cf342b3de389af940214ed46428 Gerrit-Change-Number: 7353 Gerrit-PatchSet: 3 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-Comment-Date: Sat, 30 Sep 2017 01:53:49 +0000 Gerrit-HasComments: Yes
