Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13558 )
Change subject: IMPALA-8593: Support table capabilities handling with Hive 3 ...................................................................... Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/13558/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/13558/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@92 PS7, Line 92: MAJOR_VERSION > This change doesn't seem logical to me - the major version that getMajorVer Yeah, sorry about that, I thought MAJOR_VERSION is only used to generate the variable 'impalaId' in L466. So this is rather HIVE_MAJOR_VERSION I think. setHiveClientCapabilites() is currently called in a static initializer block which is called by the Java runtime when the class MetastoreClientPool gets loaded. Maybe we should have a static method in the MetastoreShims and call it explicitly. http://gerrit.cloudera.org:8080/#/c/13558/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@532 PS7, Line 532: shim > Done sorry, hive. http://gerrit.cloudera.org:8080/#/c/13558/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/13558/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@270 PS7, Line 270: case READ: : case ANY: : default: : ensureTableSupported(table); > write only belong to the operation WRITE. In the code ensureTableSupported, Yeah, but here we want to check if the table has READ access type, right? ensureTableSupported() also succeeds if the table doesn't have READ access type, only WRITE. (might not be a typical scenario to have a write-only table, but in theory it's possible). http://gerrit.cloudera.org:8080/#/c/13558/7/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java File fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java: http://gerrit.cloudera.org:8080/#/c/13558/7/fe/src/main/java/org/apache/impala/catalog/MetaStoreClientPool.java@47 PS7, Line 47: static { : if (MetastoreShim.getMajorVersion() > 2) { : MetastoreShim.setHiveClientCapabilities(); : } : } > The setHiveClientCapabilities is only implemented in MetasoreShim 3, in Shi But if you only add it to MetastoreShim 3, then it should be fine, right? Anyway, until we use this static initializer block the Impala BackendConfig won't be initialized before setHiveClientCapabilities() is called. Maybe we should initialize the MetastoreShim explicitly. -- To view, visit http://gerrit.cloudera.org:8080/13558 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia08d01168660830b6e0d08b55a95eac129889cec Gerrit-Change-Number: 13558 Gerrit-PatchSet: 7 Gerrit-Owner: Yongzhi Chen <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Sudhanshu Arora <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Yongzhi Chen <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 12 Jul 2019 17:47:50 +0000 Gerrit-HasComments: Yes
