Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13253 )
Change subject: [IMPALA-8435] Prohibit operations on full transactional table. ...................................................................... Patch Set 2: (9 comments) What about ALTER? Both in terms of disallowing access to ACID tables, and also to prevent someone from altering the transactional table property? http://gerrit.cloudera.org:8080/#/c/13253/2/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/13253/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@139 PS2, Line 139: "Fully Acid table is not supported: "; Let's make this slightly more explanatory. Something like "Transactional (ACID) tables are only supported when they are configured as insert_only." http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@205 PS2, Line 205: public void ensureNonFullAcidTable(Map<String, String> tblProperties, does this actually use any members? perhaps it can be static and moved to AcidUtils or something as well? http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@209 PS2, Line 209: || isInsertOnlyTable(tblProperties)) { nit: kinda weird wrapping here compared to how it normally looks in Impala http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@217 PS2, Line 217: //The code is same as what exists in AcidUtils.java in hive-exec. Given this, maybe we should start our own 'AcidUtils' class where we put these copy-pasted items? Then it will be easier to locate all of the things we've copied and swap them out, vs having the code strewn about? http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@283 PS2, Line 283: analyzer.ensureNonFullAcidTable(table_.getMetaStoreTable().getParameters(), For now, we should probably also disallow INSERT into insert_only tables, until we add support http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/TableDef.java File fe/src/main/java/org/apache/impala/analysis/TableDef.java: http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/TableDef.java@225 PS2, Line 225: //Disallow creation of fullAcid table. nit: space after // nit: I don't think we should camel case 'fullAcid' because it makes it look like an identifier. Let's call it "Full ACID" or something http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/TruncateStmt.java File fe/src/main/java/org/apache/impala/analysis/TruncateStmt.java: http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/TruncateStmt.java@70 PS2, Line 70: analyzer.ensureNonFullAcidTable(table_.getMetaStoreTable().getParameters(), same here -- we shouldn't support writes to insert_only until it's supported http://gerrit.cloudera.org:8080/#/c/13253/2/testdata/datasets/functional/functional_schema_template.sql File testdata/datasets/functional/functional_schema_template.sql: http://gerrit.cloudera.org:8080/#/c/13253/2/testdata/datasets/functional/functional_schema_template.sql@2122 PS2, Line 2122: ---- BASE_TABLE_NAME Do these tables work on Hive 2? If they fail to get created, we can use a VERSION_REQUIREMENTS string, if you rebase on top of https://gerrit.cloudera.org/c/13251/2/testdata/datasets/functional/functional_schema_template.sql#2572 http://gerrit.cloudera.org:8080/#/c/13253/2/testdata/datasets/functional/functional_schema_template.sql@2124 PS2, Line 2124: ---- CREATE we should probably use CREATE_HIVE here. Otherwise wouldn't this get disallowed during data loading by your change? Also, I think we need to tweak this so we end up with an insert_only transactional table across all file formats. I think that's done by using the COLUMNS / TABLEPROPERTIES / etc sections instead of fully writing out a CREATE statement, so that the statement gets templated? -- To view, visit http://gerrit.cloudera.org:8080/13253 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I542570e30afdd8351250236d1be0077a170dd4ab Gerrit-Change-Number: 13253 Gerrit-PatchSet: 2 Gerrit-Owner: Sudhanshu Arora <sudhan...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com> Gerrit-Comment-Date: Tue, 07 May 2019 08:02:38 +0000 Gerrit-HasComments: Yes
