Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-5184: build fe against both Hive 1 & 2 APIs ......................................................................
Patch Set 7: (9 comments) http://gerrit.cloudera.org:8080/#/c/5538/7//COMMIT_MSG 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 incomplete http://gerrit.cloudera.org:8080/#/c/5538/7/bin/impala-config.sh File bin/impala-config.sh: Line 134: export IMPALA_HIVE_MAJOR_VERSION=1 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) http://gerrit.cloudera.org:8080/#/c/5538/7/common/thrift/.gitignore 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. http://gerrit.cloudera.org:8080/#/c/5538/7/common/thrift/CMakeLists.txt File common/thrift/CMakeLists.txt: Line 189: # The thrift-generated java classes defined in TCLIService are also downloaded via maven. 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? http://gerrit.cloudera.org:8080/#/c/5538/7/fe/src/compat-hive-1/java/org/apache/impala/compat/MetaStoreCompatShim.java File fe/src/compat-hive-1/java/org/apache/impala/compat/MetaStoreCompatShim.java: 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. http://gerrit.cloudera.org:8080/#/c/5538/7/fe/src/compat-hive-2/java/org/apache/impala/compat/MetaStoreCompatShim.java File fe/src/compat-hive-2/java/org/apache/impala/compat/MetaStoreCompatShim.java: 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 http://gerrit.cloudera.org:8080/5538 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifbc265281c04fe3136bc3c920dbac966742ce09a Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-HasComments: Yes
