sandeep akinapelli has posted comments on this change. Change subject: IMPALA-2636: HS2 GetTables() returns TABLE_TYPE as TABLE for VIEW ......................................................................
Patch Set 2: (13 comments) Addressed review 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"; > In any case I think returning "VIEW" is better, but it would be good to doc Done Line 62: private static final String TABLE_TYPE_VIEW = "VIEW"; > Please check if this is consistent with what Hive returns. Does Hive return Hive does return VIRTUAL_VIEW if "hive.server2.table.type.mapping" is set to "hive". I am assuming we only want to represent two types table and view. (also we are not distinguishing between managed table and external table) Line 63: private static final String TABLE_TYPE_INDEX_TABLE = "INDEX"; > Why not just "INDEX"? Yep. Index is probably simpler to understand. PS1, Line 221: taleTypes > typo Done PS1, Line 221: type > types Done Line 298: if (table.getMetaStoreTable() != null) { > Why this null check? Loaded tables should really have this set. I ran this locally and sometime I get null, not sure why! Line 321: TableType tType; > single line and space after "if" Done Line 324: try { > Is this really needed? If so, please Inline into L326. not absolutely. extra precaution since coming through thrift as a string. Inlined.. Line 328: } > indent case by 2 Done Line 338: return defaultTableType; > move this up so it's clear the switch block cannot throw Done Line 495: if (tableTypes != null && !tableTypes.isEmpty()) { > This should be fixed as well. good catch. fixed. 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 TestUnloadedView() throws ImpalaException { > We should test two cases: Done Line 226: "create view %s.test_view as select * from functional.alltypes", dbName)); > No need to test tables here Done -- 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: sandeep akinapelli <[email protected]> Gerrit-HasComments: Yes
