Zach Amsden has posted comments on this change. Change subject: IMPALA-4318: Kudu support for CREATE EXTERNAL TABLE AS SELECT ......................................................................
Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/6261/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: PS1, Line 1033: CreateTableStmt.CreateTableAsSelectStmt > maybe I'll see why this is necessary later, but this seems odd since it loo The name is confusing. It's just a static factory method for creating a CreatTableStmt that is going to be used as a sub-part of a CreateTableAsSelectStmt (and it needs to know that). http://gerrit.cloudera.org:8080/#/c/6261/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java: PS1, Line 68: public static CreateTableStmt CreateTableAsSelectStmt(TableDef tableDef) { : CreateTableStmt stmt = new CreateTableStmt(tableDef); : stmt.createAsSelect_ = true; : return stmt; : } > yeah, per my comment in sql-parser this seems odd to have this parameter on I didn't want to override the constructor for CreateTableStmt with a boolean parameter 'isCreateAsSelect' - in 2 years nobody is going to know why some boolean true is passed or what it does and it isn't clear from the calling context. So instead I created a static factory method for CreateTableStmt that tells you exactly what it is doing. Perhaps a more terse name would be better and it does look confusing that it is named the same as another statement. I can't just set isExternal = true because that can also be true without the context of CTAS. We need to know whether this table is to be created from a select, or have column data ingested from an external table. The fact that it is an external table is now orthogonal to that. -- To view, visit http://gerrit.cloudera.org:8080/6261 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9aa82809a6c0c5e6386827314b7e5b520c1a6633 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
