impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5184: build fe against both Hive 1 & 2 APIs
Date Mon, 10 Apr 2017 20:04:46 GMT
Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-5184: build fe against both Hive 1 & 2 APIs

Patch Set 7:

Commit Message:

Line 9: This adds a compatibility shim layer with Hive 1 and Hive 2
Is there a Hive doc summarizing the compatibility changes. Did a quick web search, couldn't
find any.

PS7, Line 16: the
File bin/

This can be inferred from IMPALA_HIVE_VERSION given the naming scheme is followed? (Unless
we want to support a combination of different IMPALA_HIVE_MAJOR_VERSION and IMPALA_HIVE_VERSION,
which I don't think is the case)
File common/thrift/.gitignore:

Line 4: hive-2-api/TCLIService.thrift
May be add hive-*-api/TCLIService.thrift (Not sure the exact wildcard syntax works for .gitignore)?
I don't think we are expected to change Hive's spec anyway.
File common/thrift/CMakeLists.txt:

Line 189: # The thrift-generated java classes defined in TCLIService are also downloaded via
Wondering why this logic is required instead of adding a copy of hive-2-api/TCLIService.thrift
from hive-2's git repo just like how we maintain a copy of hive-1-api/....thrift. Whoever
is compiling can just toggle IMPALA_HIVE_MAJOR_VERSION and use the appropriate thrift file.
Am I missing something?
File fe/src/compat-hive-1/java/org/apache/impala/compat/

PS7, Line 46: MetaStoreCompatShim
MetaStoreCompatShim sounds kinda redundant, maybe just MetaStoreShim? I'm not too strong about
this, up to you :).

PS7, Line 68:           
4 space indentation, here and multiple places in this file.
File fe/src/compat-hive-2/java/org/apache/impala/compat/

Line 43:  * A wrapper around some of Hive's metastore API's to abstract away differences
May be add the versions this shim is compatible for? We can get it from the package name,
but I think it is worthy of a class comment (Here and the other shim too).

Line 100:   public static TResultSet execGetFunctions(Frontend frontend,
Do these belong here? They look similar across both the Shim classes.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbc265281c04fe3136bc3c920dbac966742ce09a
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-HasComments: Yes

View raw message