impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-2636: HS2 GetTables() returns TABLE TYPE as TABLE for VIEW
Date Tue, 12 Sep 2017 00:34:05 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-2636: HS2 GetTables() returns TABLE_TYPE as TABLE for VIEW

Patch Set 1:


Nice, thanks for working on this one
File fe/src/main/java/org/apache/impala/service/

Line 62:   private static final String TABLE_TYPE_VIEW = "VIEW";
Please check if this is consistent with what Hive returns. Does Hive return "VIRTUAL_VIEW"?

Line 63:   private static final String TABLE_TYPE_INDEX_TABLE = "INDEX_TABLE";
Why not just "INDEX"?
(Not that it really matters too much since Impala cannot load these anyway)

Line 298:             if (table.getMetaStoreTable() != null) {
Why this null check? Loaded tables should really have this set.

Line 321:     if(typeStr == null) {
single line and space after "if"

Line 324:     typeStr = typeStr.toUpperCase();
Is this really needed? If so, please Inline into L326.

Line 328:       case EXTERNAL_TABLE:
indent case by 2

Line 338:     } catch (Exception e) {
move this up so it's clear the switch block cannot throw

Line 495:     // Impala catalog only contains TABLE. Returns an empty set if the search does
This should be fixed as well.
File fe/src/test/java/org/apache/impala/service/

Line 221:   public void TestGetView() throws ImpalaException {
We should test two cases:
1. View that is not loaded is shown as a table
2. View that is loaded is shown as a view

For testing the first case you should be able to use addTestDb() and addTestView()

For testing the second case, add a dummy AnalyzesOk("select * from functional.alltypes_view")
to make sure the view is really loaded at this point - otherwise the test relies on the view
already being loaded and might be flaky

Line 226:     req.get_tables_req.setSchemaName("functional");
No need to test tables here

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I90616388e6181cf342b3de389af940214ed46428
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-HasComments: Yes

View raw message