Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201709192349.v8JNnrb2022465@ip-10-146-233-104.ec2.internal>
Date: Tue, 19 Sep 2017 23:49:53 +0000
From: "Alex Behm (Code Review)" <gerrit@cloudera.org>
To: sandeep akinapelli <sakinapelli@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mjacobs@apache.org>,
        Vuk Ercegovac <vercegovac@cloudera.com>
Reply-To: alex.behm@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-2636=3A_HS2_GetTables=28=29_returns_TABLE_TYPE_as_TABLE_for_VIEW=0A?=
In-Reply-To: <gerrit.1499124183000.I90616388e6181cf342b3de389af940214ed46428@gerrit.cloudera.org>
References: <gerrit.1499124183000.I90616388e6181cf342b3de389af940214ed46428@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Tue, 19 Sep 2017 23:49:59 -0000

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