Tim Armstrong has posted comments on this change. Change subject: IMPALA-5184: build fe against both Hive 1 & 2 APIs ......................................................................
Patch Set 7: (11 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 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 Done 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 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. 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 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. 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 adds additional RPC endpoints that we don't implement. 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 n Done PS7, Line 68: > 4 space indentation, here and multiple places in this file. Done 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 Done 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. http://gerrit.cloudera.org:8080/#/c/5538/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: Line 3092: } Longline -- 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 <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
