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 2: (12 comments) Patch 3 will address the issues. 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(); > Just to be clear I was saying to do the following These strings are only used to be packed into a string array and sent to hive as capabilities. I will use enum in the future if we need enum operations. I will remove the intern (just copied from hive code) http://gerrit.cloudera.org:8080/#/c/13558/1/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@92 PS1, Line 92: private static final long MAJOR_VERSION = 3; > nit: HIVE_MAJOR_VERSION? It is Impala shim's major version. http://gerrit.cloudera.org:8080/#/c/13558/1/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@437 PS1, Line 437: * Set impala capabilities to hive client > nit: could you elaborate a little? E.g. why is it needed, what are these ca Done 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_); > 1) If Hive does not define Metadata_modification and we want to fold this o Bitwise can combine two checks into one (for example check write along with read/write). The major purpose is to integrate with hive's translation layer, hive access type is in byte, bitwise operations work fine with it and may have better performance and simpler (save a couple of switch statements) This function is frequently called, checking annotation value for operation type may be roundabout. I will accept your first option to use enum . 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; > Why not import hive_metastoreConstants access types? It is auto-generated code and Hive 2 does not have these constant. We do not want to break hive 2 impala. http://gerrit.cloudera.org:8080/#/c/13558/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@150 PS1, Line 150: "Table %s not supported. Bucketed tables are only supported " + : "for read." > nit: How about "%s is a bucketed table. Only read operations are supported Done http://gerrit.cloudera.org:8080/#/c/13558/1/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java: http://gerrit.cloudera.org:8080/#/c/13558/1/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@876 PS1, Line 876: if (MetastoreShim.getMajorVersion() > 2) { > Maybe you could add tests for the other statements as well, e.g. COMPUTE ST Done http://gerrit.cloudera.org:8080/#/c/13558/1/testdata/datasets/functional/functional_schema_template.sql File testdata/datasets/functional/functional_schema_template.sql: http://gerrit.cloudera.org:8080/#/c/13558/1/testdata/datasets/functional/functional_schema_template.sql@2183 PS1, Line 2183: ---- LOAD : INSERT OVERWRITE TABLE {db_name}{db_suffix}.{table_name} : SELECT id, int_col from functional.alltypes; > This will only run for the non-text versions of the tables. If you want the Done http://gerrit.cloudera.org:8080/#/c/13558/2/testdata/datasets/functional/functional_schema_template.sql File testdata/datasets/functional/functional_schema_template.sql: http://gerrit.cloudera.org:8080/#/c/13558/2/testdata/datasets/functional/functional_schema_template.sql@2172 PS2, Line 2172: CLUSTERED BY (col1) INTO 5 BUCKETS > Without STORED AS {file_format} all tables will be text files. Done http://gerrit.cloudera.org:8080/#/c/13558/2/testdata/datasets/functional/functional_schema_template.sql@2173 PS2, Line 2173: LOCATION '/test-warehouse/{table_name}'; > Adding {db_name}{db_suffix} would ensure that tables in different databases Done http://gerrit.cloudera.org:8080/#/c/13558/2/testdata/datasets/functional/functional_schema_template.sql@2182 PS2, Line 2182: CLUSTERED BY (col1) INTO 5 BUCKETS; > Same as line 2172. Done http://gerrit.cloudera.org:8080/#/c/13558/2/testdata/datasets/functional/functional_schema_template.sql@2183 PS2, Line 2183: ---- LOAD : INSERT OVERWRITE TABLE {db_name}{db_suffix}.{table_name} : SELECT id, int_col from functional.alltypes; > This only loads the data to the text table, the other formats won't contain 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: 2 Gerrit-Owner: Yongzhi Chen <yc...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Sudhanshu Arora <sudhan...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Wed, 12 Jun 2019 18:15:25 +0000 Gerrit-HasComments: Yes
