Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10720 )
Change subject: IMPALA-6305: Allow column definitions in ALTER VIEW ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java: http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@36 PS2, Line 36: TableName tableName, ArrayList<ColumnDef> columnDefs, QueryStmt viewDefStmt) { > Use List<ColumnDef> instead of ArrayList<ColumnDef> Likely extends to the super class. If so, would be good to fix that as well. http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@44 PS2, Line 44: viewDefStmt_.analyze(analyzer); have a look at CreateViewStmt's analyze. would be useful to double-check the delta-- might indicate a test gap or perhaps the create view does more than needed? couple of diffs to note: - complex typed columns - accessEvent? http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@69 PS2, Line 69: } add the column defs? see CreateViewStmt is there a test for this? http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1094 PS2, Line 1094: + "select int_col x, string_col y from functional.alltypes"); does "*" in the select also work? you might increase coverage by piggy-backing the alter view with existing create view tests (got the idea since the two have a common parent class) http://gerrit.cloudera.org:8080/#/c/10720/2/testdata/workloads/functional-query/queries/QueryTest/views-ddl.test File testdata/workloads/functional-query/queries/QueryTest/views-ddl.test: http://gerrit.cloudera.org:8080/#/c/10720/2/testdata/workloads/functional-query/queries/QueryTest/views-ddl.test@234 PS2, Line 234: abc comment 'agg', xyz comment 'gby' these verify that the new syntax is admitted. can you also check that columns can be renamed (and verified via describe and a query)? -- To view, visit http://gerrit.cloudera.org:8080/10720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6073444a814a24d97e80df15fcd39be2812f63fc Gerrit-Change-Number: 10720 Gerrit-PatchSet: 2 Gerrit-Owner: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Thu, 14 Jun 2018 23:12:49 +0000 Gerrit-HasComments: Yes
