Sudhanshu Arora has posted comments on this change. ( http://gerrit.cloudera.org:8080/13253 )
Change subject: [IMPALA-8435] Prohibit operations on full transactional table. ...................................................................... Patch Set 3: (8 comments) 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: "Table %s not supported.Transactional (ACID) tables are " + > Let's make this slightly more explanatory. Something like "Transactional (A Done http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@205 PS2, Line 205: tableName)); > does this actually use any members? perhaps it can be static and moved to A Done http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@209 PS2, Line 209: // State shared between all objects of an Analyzer tree. We use LinkedHashMap and > nit: kinda weird wrapping here compared to how it normally looks in Impala Done http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@217 PS2, Line 217: public final AuthorizationFactory authzFactory; > Given this, maybe we should start our own 'AcidUtils' class where we put th Done http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@221 PS2, Line 221: > From hive code, is assume parameters can be null, for example: Done. I don't know when can this be null but added the null check to be safe. I see code in Hive that is setting the params explicitly to null. http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java File fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java: http://gerrit.cloudera.org:8080/#/c/13253/2/fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java@67 PS2, Line 67: analyzer.ensureNonFullAcidTable(getTable().getMetaStoreTable().getParameters(), > Most ensureNonFullAcidTable is in this style, is that worth to add a new me Done 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: usteringCols = isHBaseTable > I think that analyzeTargetTable() would be a more logical place for the che Done 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@563 PS2, Line 563: AnalyzesOk("select * from functional.insert_only_transactional_table"); : > Duplicate line. Done -- 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: 3 Gerrit-Owner: Sudhanshu Arora <[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-Comment-Date: Fri, 10 May 2019 22:12:50 +0000 Gerrit-HasComments: Yes
