Csaba Ringhofer 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: (6 comments) http://gerrit.cloudera.org:8080/#/c/13253/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13253/2//COMMIT_MSG@7 PS2, Line 7: [IMPALA-8435] Impala commits use the following format most of the time: IMPALA-XXXX: title http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java: http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java@165 PS2, Line 165: // Make sure the source table exists and the user has permission to access it. : FeTable srcTable = analyzer.getTable(srcTableName_, Privilege.VIEW_METADATA); : : analyzer.ensureNonFullAcidTable(srcTable.getMetaStoreTable().getParameters(), : srcTable.getFullName()); I am not sure whether it would be a good idea, but most statements could be covered by moving the check to analyzer.getTable() and interpret the requested privilege as intention, and only allow the operation for full acid tables if it is Privilege.VIEW_METADATA or Privilege.DROP. 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@284 PS2, Line 284: targetTableName_.toString()) I think that analyzeTargetTable() would be a more logical place for the check. The authorization check is there, and the pattern in other statements seem to be that ensureNonFullAcidTable() is called not too far after auth check. http://gerrit.cloudera.org:8080/#/c/13253/2/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/13253/2/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@534 PS2, Line 534: TestAnalyzeTransactional some more statements that could be tested: show create table create table ... TBLPROPERTIES('transactional'='true') show column stats http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@536 PS2, Line 536: "Fully Acid table is not supported: functional.full_transactional_table" Creating a string with the error message could make the tests more compact. http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@563 PS2, Line 563: AnalyzesOk("describe functional.insert_only_transactional_table"); : AnalyzesOk("describe functional.insert_only_transactional_table"); Duplicate line. -- 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 12:08:54 +0000 Gerrit-HasComments: Yes
