impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5184: build fe against both Hive 1 & 2 APIs
Date Mon, 10 Apr 2017 22:35:12 GMT
Tim Armstrong 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 
I don't believe so. I looked at the history of some of these changes and couldn't find an
overall list.

PS7, Line 16: the
> incomplete
File bin/

> This can be inferred from IMPALA_HIVE_VERSION given the naming scheme is fo
Yeah we probably don't need the flexibility.

Line 377: export HIVE_SRC_DIR=${HIVE_SRC_DIR:-"${HIVE_HOME}/src"}
I ran into the dreaded sticky config variable problem with some of these variables switching
between versions. This seems like a good time to fix it before more Impala devs end up switching
between versions.
File common/thrift/.gitignore:

Line 4: hive-2-api/TCLIService.thrift
> May be add hive-*-api/TCLIService.thrift (Not sure the exact wildcard synta
The other one is checked in, so any changes to it shouldn't be ignored.

Hive's TCLIService has had various additions to it, so it may make sense at some point to
refresh this.
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 adds additional RPC endpoints that we don't implement.
File fe/src/compat-hive-1/java/org/apache/impala/compat/

PS7, Line 46: MetaStoreCompatShim
> MetaStoreCompatShim sounds kinda redundant, maybe just MetaStoreShim? I'm n

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

Line 100:   public static TResultSet execGetFunctions(Frontend frontend,
> Do these belong here? They look similar across both the Shim classes.
The thrift classes moved between packages. The code is identical but the classes moved from
the cli to rpc package so the imports up the top of the file are different. There isn't really
a good way to do conditional imports in Java.
File fe/src/main/java/org/apache/impala/service/

Line 3092:         }

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-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message