Yongzhi Chen 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 9: (10 comments) Patch set 9 addresses review issues and adds more tests. 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 EXTWRITE = "EXTWRITE"; : private static final String EXTREAD = "EXTREAD"; > nit: unused Done 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 HIVEMANAGEDINSERTREAD = "HIVEMANAGEDINSERTREAD"; : private static final String HIVEMANAGEDINSERTWRITE = "HIVEMANAGEDINS > nit: unused these are some possible capabilities we may support. 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 HIVEMQT = "HIVEMQT"; > nit: unused We may need it in the future. http://gerrit.cloudera.org:8080/#/c/13558/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@92 PS7, Line 92: eUtils.valida > nit: Technically this class doesn't have a version number, so maybe we shou Done http://gerrit.cloudera.org:8080/#/c/13558/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@470 PS7, Line 470: HIVESQL, > nit: maybe add TODO for HIVEMANAGEDINSERTWRITE once IMPALA-8636 goes in. Done http://gerrit.cloudera.org:8080/#/c/13558/7/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@532 PS7, Line 532: > nit: impala Done 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 ACCESSTYPE_READ = (byte)2; : public static final byte ACCESSTYPE_WRITE = (byte)4; : public static final byte ACCESSTYPE_READWRITE = (byte > nit: maybe we could use the same naming as in Hive, i.e. ACCESSTYPE_READONL Done 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_WRITE write only belong to the operation WRITE. In the code ensureTableSupported, require_write(write only) is already included. 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? The setHiveClientCapabilities is only implemented in MetasoreShim 3, in Shim2 the method just throws Unsupported exception. add the if statement to avoid thrown unsupported Exception in Hive 2 http://gerrit.cloudera.org:8080/#/c/13558/7/tests/metadata/test_ddl.py File tests/metadata/test_ddl.py: http://gerrit.cloudera.org:8080/#/c/13558/7/tests/metadata/test_ddl.py@669 PS7, Line 669: if HIVE_MAJOR_VERSION > 2: : assert properties['OBJCAPABILITIE > We could also check OBJCAPABILITIES here, as it should have a fix values. Done -- 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: 9 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 14:49:13 +0000 Gerrit-HasComments: Yes
