Yongzhi Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/13558 )
Change subject: IMPALA-8593: Prohibit write operations for bucketed tables ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/13558/1/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/1/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@78 PS1, Line 78: private static final String CONNECTORREAD = "CONNECTORREAD".intern(); > How about using enum for this? These are hive defined strings for capabilities, they only recognize these strings. http://gerrit.cloudera.org:8080/#/c/13558/1/fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java: http://gerrit.cloudera.org:8080/#/c/13558/1/fe/src/main/java/org/apache/impala/analysis/AlterTableStmt.java@93 PS1, Line 93: analyzer.ensureTableWriteSupported(table_); > Nit: Another way to do this is to introduce another enum say OperationType. Hive defines access types as byte, we combine the access type check in MetastoreShim and try not to spread the definition to so many places and take advantage of bitwise operations. There is no METADATA_MODIFICATION in Hive, only read, write, read/write: We need the write operation(write or read/write) for it, this method includes both. http://gerrit.cloudera.org:8080/#/c/13558/1/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/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@129 PS1, Line 129: public static final byte REQUIRE_READ = (byte)2; > If we create an enum then these values can go into enum itself They are consistent with hive thrift defined constant for access type, and byte type has one more advantage: it can run bitwise operations like | (bitwise logical or) http://gerrit.cloudera.org:8080/#/c/13558/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: http://gerrit.cloudera.org:8080/#/c/13558/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@4021 PS1, Line 4021: ParserError("Create table bucketed_tbl(order_id int, order_name string)" > Wouldn't this fail during analysis? Parsing happens before analysis. Impala does not recognize clustered by, so it failed with the Syntax error -- 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: 1 Gerrit-Owner: Yongzhi Chen <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[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: Tue, 11 Jun 2019 12:02:32 +0000 Gerrit-HasComments: Yes
