impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2636: HS2 GetTables() returns TABLE TYPE as TABLE for VIEW
Date Tue, 19 Sep 2017 23:49:53 GMT
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 <sakinapelli@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mjacobs@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <vercegovac@cloudera.com>
Gerrit-Reviewer: sandeep akinapelli <sakinapelli@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message