Alex Behm has posted comments on this change. Change subject: IMPALA-3237: Disallow inserting into tables with unsupported types ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/2775/1/fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java File fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java: Line 293: if (!colDef.getType().isSupported()) { nit: move above the duplicate check? seems like we'd like to see unsupported first Line 297: } I think we should also forbid ALTER TABLE that would change a column to an unsupported type. If you prefer we could separate out those changes: Patch 1: Only forbid INSERT Patch 2: Forbid CREATE, CTAS, ALTER, SELECT *, etc. http://gerrit.cloudera.org:8080/#/c/2775/1/fe/src/main/java/com/cloudera/impala/analysis/InsertStmt.java File fe/src/main/java/com/cloudera/impala/analysis/InsertStmt.java: Line 347: for (Column c :table_.getColumns()) { style: the colon should come after "c" Line 350: "'%s' because the column '%s' has an unsupported type '%s'.", can you make the error msg as consistent as possible with the other ones? for example, in the other err msgs we use (%s) for the target table http://gerrit.cloudera.org:8080/#/c/2775/1/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java: Line 1391: AnalysisError("create table new_table (i binary)", Might as well write a loop to test this over all unsupported types. -- To view, visit http://gerrit.cloudera.org:8080/2775 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2e218ad8ec303172e56e2002d0bee2f1edf83fe0 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-HasComments: Yes
