impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "sandeep akinapelli (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2636: HS2 GetTables() returns TABLE TYPE as TABLE for VIEW
Date Sat, 30 Sep 2017 01:53:49 GMT
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 <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-Comment-Date: Sat, 30 Sep 2017 01:53:49 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message