Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/10720 )
Change subject: IMPALA-6305: Allow column definitions in ALTER VIEW ...................................................................... Patch Set 4: (18 comments) Hey Vuk, Fredy, I am sorry I couldn't address your comments earlier. I was caught up with bootcamp last week. I have addressed all your comments. I have also created an issue (IMPALA-7209) to keep track of the bug that impala allows self referencing alter view statements. Thanks, Pooja http://gerrit.cloudera.org:8080/#/c/10720/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10720/2//COMMIT_MSG@11 PS2, Line 11: and the AlterViewStmt constructor. > Add an example of the new SQL syntax. Done 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: */ > Likely extends to the super class. If so, would be good to fix that as well Done http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@44 PS2, Line 44: public void analyze(Analyzer analyzer) throws AnalysisException { > have a look at CreateViewStmt's analyze. would be useful to double-check th Done http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@69 PS2, Line 69: public String toSql() { > add the column defs? see CreateViewStmt Done. There is no test for this as of now. 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@1091 PS2, Line 1091: " > nit: looking at the style of other code in this file, we use + and then new Done 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? Done http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1101 PS2, Line 1101: AnalyzesOk("alter view functional.alltypes_view (aaa, bbb) as " > Add a test for altering a view with the same column as the existing one. Done http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1105 PS2, Line 1105: AnalyzesOk("alter view functional.complex_view (abc, xyz) as " > Can we alter a view from its own? As we discussed, "alter view functional.alltypes_view (new_int_col) as select int_col from functional.alltypes_view" is actually invalid SQL. Currently the master branch also lets you alter a view referencing itself. I have created a ticket for it. I will fix that issue separately. http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1146 PS2, Line 1146: "sel > nit: remove indentation Done http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1148 PS2, Line 1148: isma > nit: remove indentation Done http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1150 PS2, Line 1150: "sel > nit: remove indentation Done http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1152 PS2, Line 1152: "vie > nit: remove indentation Done http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1155 PS2, Line 1155: "Col > nit: remove indentation Done http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1156 PS2, Line 1156: "vie > nit: remove indentation Done http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1160 PS2, Line 1160: "inn > nit: remove indentation Done 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: aaa comment 'abc', bbb comment 'xyz' > these verify that the new syntax is admitted. can you also check that colum Done http://gerrit.cloudera.org:8080/#/c/10720/2/testdata/workloads/functional-query/queries/QueryTest/views-ddl.test@241 PS2, Line 241: 'View has been altered.' > Can we also verify the altered view? Done http://gerrit.cloudera.org:8080/#/c/10720/2/testdata/workloads/functional-query/queries/QueryTest/views-ddl.test@248 PS2, Line 248: 'bbb','string','xyz' > Verify the altered view. Done -- 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: 4 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: Mon, 25 Jun 2018 18:17:01 +0000 Gerrit-HasComments: Yes
