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: Code-Review+1 (10 comments) Lgtm, found some nits, but nothing serious. 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@78 PS7, Line 78: private static final String CONNECTORREAD = "CONNECTORREAD"; : private static final String CONNECTORWRITE = "CONNECTORWRITE"; nit: unused http://gerrit.cloudera.org:8080/#/c/13558/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@83 PS7, Line 83: private static final String HIVEFULLACIDREAD = "HIVEFULLACIDREAD"; : private static final String HIVEFULLACIDWRITE = "HIVEFULLACIDWRITE"; nit: unused http://gerrit.cloudera.org:8080/#/c/13558/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@87 PS7, Line 87: private static final String HIVEMANAGESTATS = "HIVEMANAGESTATS"; nit: unused 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 nit: Technically this class doesn't have a version number, so maybe we should rename it to IMPALA_MAJOR_VERSION http://gerrit.cloudera.org:8080/#/c/13558/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@470 PS7, Line 470: HIVEMANAGEDINSERTREAD, nit: maybe add TODO for HIVEMANAGEDINSERTWRITE once IMPALA-8636 goes in. 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 nit: impala 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@130 PS7, Line 130: public static final byte REQUIRE_READ = (byte)2; : public static final byte REQUIRE_WRITE = (byte)4; : public static final byte REQUIRE_READWRITE = (byte)8; nit: maybe we could use the same naming as in Hive, i.e. ACCESSTYPE_READONLY, etc. http://gerrit.cloudera.org:8080/#/c/13558/7/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@198 PS7, Line 198: OperationType nit: please add comment 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); nit: ensureTableSupported() also succeeds if the table has ACCESSTYPE_WRITEONLY, however having a write only table might not be a real life scenario. 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(); : } : } nit: Can you place this to MetastoreShim 3 only? static { setHiveClientCapabilities(); } -- 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: Thu, 11 Jul 2019 17:45:03 +0000 Gerrit-HasComments: Yes
