[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10720 ) Change subject: IMPALA-6305: Allow column definitions in ALTER VIEW .. IMPALA-6305: Allow column definitions in ALTER VIEW This change adds support to change column definitions in ALTER VIEW statements. This support only required minor changes in the parser and the AlterViewStmt constructor. Here's an example syntax: alter view foo (a, b comment 'helloworld') as select * from bar; describe foo; +--+++ | name | type | comment| +--+++ | a| string || | b| string | helloworld | +--+++ The following tests were modified: 1. ParserTest - To check that the parser handles column definitions for alter view statements. 2. AnalyzerDDLTest - To ensure that the analyzer supports the change column definitions parsed. 3. TestDdlStatements - To verify the end-to-end functioning of ALTER VIEW statements with change column definitions. 4. AuthorizationTest - To ensure that alter table commands with column definitions check permissions as expected. Change-Id: I6073444a814a24d97e80df15fcd39be2812f63fc Reviewed-on: http://gerrit.cloudera.org:8080/10720 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M testdata/workloads/functional-query/queries/QueryTest/views-ddl.test 8 files changed, 148 insertions(+), 11 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- 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: merged Gerrit-Change-Id: I6073444a814a24d97e80df15fcd39be2812f63fc Gerrit-Change-Number: 10720 Gerrit-PatchSet: 9 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10720 ) Change subject: IMPALA-6305: Allow column definitions in ALTER VIEW .. Patch Set 8: Verified+1 -- 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: 8 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 27 Jun 2018 03:41:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10720 ) Change subject: IMPALA-6305: Allow column definitions in ALTER VIEW .. Patch Set 8: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2750/ DRY_RUN=false -- 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: 8 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 27 Jun 2018 00:13:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10720 ) Change subject: IMPALA-6305: Allow column definitions in ALTER VIEW .. Patch Set 8: Code-Review+2 -- 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: 8 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 27 Jun 2018 00:13:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
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 7: Code-Review+2 -- 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: 7 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 26 Jun 2018 23:57:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
Pooja Nilangekar has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/10720 ) Change subject: IMPALA-6305: Allow column definitions in ALTER VIEW .. IMPALA-6305: Allow column definitions in ALTER VIEW This change adds support to change column definitions in ALTER VIEW statements. This support only required minor changes in the parser and the AlterViewStmt constructor. Here's an example syntax: alter view foo (a, b comment 'helloworld') as select * from bar; describe foo; +--+++ | name | type | comment| +--+++ | a| string || | b| string | helloworld | +--+++ The following tests were modified: 1. ParserTest - To check that the parser handles column definitions for alter view statements. 2. AnalyzerDDLTest - To ensure that the analyzer supports the change column definitions parsed. 3. TestDdlStatements - To verify the end-to-end functioning of ALTER VIEW statements with change column definitions. 4. AuthorizationTest - To ensure that alter table commands with column definitions check permissions as expected. Change-Id: I6073444a814a24d97e80df15fcd39be2812f63fc --- M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M testdata/workloads/functional-query/queries/QueryTest/views-ddl.test 8 files changed, 148 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/10720/7 -- 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: newpatchset Gerrit-Change-Id: I6073444a814a24d97e80df15fcd39be2812f63fc Gerrit-Change-Number: 10720 Gerrit-PatchSet: 7 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
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 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10720/3/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/3/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@76
PS3, Line 76: if (columnDefs_ != null) sb.append("(" + Joiner.on(",
").join(columnDefs_) + ".");
> As of now we don't have test to print the SQL for create or alter view stat
ok, pls add several cases. this was caught by code inspection, so will rot
as-is. until we decide to disable or remove it, lets capture some basics in
analysis.ToSqlTest.java
http://gerrit.cloudera.org:8080/#/c/10720/6/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/6/testdata/workloads/functional-query/queries/QueryTest/views-ddl.test@279
PS6, Line 279: *
explicitly list the column using the name in the 'alter' stmt so that there
isn't a false positive (who knows, perhaps the prev. col name is stuck
somewhere)
--
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: 6
Gerrit-Owner: Pooja Nilangekar
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Pooja Nilangekar
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Mon, 25 Jun 2018 22:01:55 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10720 ) Change subject: IMPALA-6305: Allow column definitions in ALTER VIEW .. Patch Set 6: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/10720/3/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/3/testdata/workloads/functional-query/queries/QueryTest/views-ddl.test@234 PS3, Line 234: comment 'abc' > Actually, the describe statement does show the comments. I am currently ch Sorry I missed that. This is good enough. -- 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: 6 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Mon, 25 Jun 2018 19:02:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
Pooja Nilangekar has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/10720 ) Change subject: IMPALA-6305: Allow column definitions in ALTER VIEW .. IMPALA-6305: Allow column definitions in ALTER VIEW This change adds support to change column definitions in ALTER VIEW statements. This support only required minor changes in the parser and the AlterViewStmt constructor. Here's an example syntax: alter view foo (a, b comment 'helloworld') as select * from bar; describe foo; +--+++ | name | type | comment| +--+++ | a| string || | b| string | helloworld | +--+++ The following tests were modified: 1. ParserTest - To check that the parser handles column definitions for alter view statements. 2. AnalyzerDDLTest - To ensure that the analyzer supports the change column definitions parsed. 3. TestDdlStatements - To verify the end-to-end functioning of ALTER VIEW statements with change column definitions. 4. AuthorizationTest - To ensure that alter table commands with column definitions check permissions as expected. Change-Id: I6073444a814a24d97e80df15fcd39be2812f63fc --- M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M testdata/workloads/functional-query/queries/QueryTest/views-ddl.test 8 files changed, 148 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/10720/6 -- 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: newpatchset Gerrit-Change-Id: I6073444a814a24d97e80df15fcd39be2812f63fc Gerrit-Change-Number: 10720 Gerrit-PatchSet: 6 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
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 5:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/10720/3/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/3/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@76
PS3, Line 76: if (columnDefs_ != null) sb.append("(" + Joiner.on(",
").join(columnDefs_) + ".");
> I don't know if we have an existing test to print the SQL. If we do, we sho
As of now we don't have test to print the SQL for create or alter view
statements.
http://gerrit.cloudera.org:8080/#/c/10720/3/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/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1102
PS3, Line 1102: +
> nit: + before a new line
Done
http://gerrit.cloudera.org:8080/#/c/10720/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1106
PS3, Line 1106: +
> nit: + before a new line
Done
http://gerrit.cloudera.org:8080/#/c/10720/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1110
PS3, Line 1110: +
> nit: + before a new line
Done
http://gerrit.cloudera.org:8080/#/c/10720/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1159
PS3, Line 1159: "sel
> nit: fix indentation
Done
http://gerrit.cloudera.org:8080/#/c/10720/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1160
PS3, Line 1160: "inn
> nit: fix indentation
Done
http://gerrit.cloudera.org:8080/#/c/10720/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java:
http://gerrit.cloudera.org:8080/#/c/10720/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1660
PS3, Line 1660: AuthzOk("ALTER VIEW functional.alltypes_view (a, b, c) as "
+
> nit: is this a tab character? If yes, please use spaces?
Done
http://gerrit.cloudera.org:8080/#/c/10720/3/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/3/testdata/workloads/functional-query/queries/QueryTest/views-ddl.test@234
PS3, Line 234: comment 'abc'
> Unfortunately "describe" doesn't show comments. "describe formatted/extende
Actually, the describe statement does show the comments. I am currently
checking it in line 246 - 248. I verified that other tests in views-ddl.test
also use describe to show the column name, type and comment. Should I
explicitly change it for this test and the rest of the tests?
--
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: 5
Gerrit-Owner: Pooja Nilangekar
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Pooja Nilangekar
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Mon, 25 Jun 2018 18:39:42 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
Pooja Nilangekar has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/10720 ) Change subject: IMPALA-6305: Allow column definitions in ALTER VIEW .. IMPALA-6305: Allow column definitions in ALTER VIEW This change adds support to change column definitions in ALTER VIEW statements. This support only required minor changes in the parser and the AlterViewStmt constructor. Here's an example syntax: alter view foo (a, b comment 'helloworld') as select * from bar; describe foo; +--+++ | name | type | comment| +--+++ | a| string || | b| string | helloworld | +--+++ The following tests were modified: 1. ParserTest - To check that the parser handles column definitions for alter view statements. 2. AnalyzerDDLTest - To ensure that the analyzer supports the change column definitions parsed. 3. TestDdlStatements - To verify the end-to-end functioning of ALTER VIEW statements with change column definitions. 4. AuthorizationTest - To ensure that alter table commands with column definitions check permissions as expected. Change-Id: I6073444a814a24d97e80df15fcd39be2812f63fc --- M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M testdata/workloads/functional-query/queries/QueryTest/views-ddl.test 8 files changed, 148 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/10720/5 -- 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: newpatchset Gerrit-Change-Id: I6073444a814a24d97e80df15fcd39be2812f63fc Gerrit-Change-Number: 10720 Gerrit-PatchSet: 5 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
Fredy Wijaya 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:
(9 comments)
LGTM overall, I just have couple nits and I can give a +1.
http://gerrit.cloudera.org:8080/#/c/10720/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10720/3//COMMIT_MSG@26
PS3, Line 26:
nit: remove spaces to not overly indented.
http://gerrit.cloudera.org:8080/#/c/10720/3/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/3/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@76
PS3, Line 76: if (columnDefs_ != null) sb.append("(" + Joiner.on(",
").join(columnDefs_) + ".");
I don't know if we have an existing test to print the SQL. If we do, we should
also update it.
http://gerrit.cloudera.org:8080/#/c/10720/3/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/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1102
PS3, Line 1102: +
nit: + before a new line
http://gerrit.cloudera.org:8080/#/c/10720/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1106
PS3, Line 1106: +
nit: + before a new line
http://gerrit.cloudera.org:8080/#/c/10720/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1110
PS3, Line 1110: +
nit: + before a new line
http://gerrit.cloudera.org:8080/#/c/10720/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1159
PS3, Line 1159: "sel
nit: fix indentation
http://gerrit.cloudera.org:8080/#/c/10720/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1160
PS3, Line 1160: "inn
nit: fix indentation
http://gerrit.cloudera.org:8080/#/c/10720/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java:
http://gerrit.cloudera.org:8080/#/c/10720/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1660
PS3, Line 1660: AuthzOk("ALTER VIEW functional.alltypes_view (a, b, c) as "
+
nit: is this a tab character? If yes, please use spaces?
http://gerrit.cloudera.org:8080/#/c/10720/3/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/3/testdata/workloads/functional-query/queries/QueryTest/views-ddl.test@234
PS3, Line 234: comment 'abc'
Unfortunately "describe" doesn't show comments. "describe formatted/extended"
does. It would be good to be able to verify the updated comments.
--
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
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Pooja Nilangekar
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Mon, 25 Jun 2018 18:17:57 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
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
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
Pooja Nilangekar has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/10720 ) Change subject: IMPALA-6305: Allow column definitions in ALTER VIEW .. IMPALA-6305: Allow column definitions in ALTER VIEW This change adds support to change column definitions in ALTER VIEW statements. This support only required minor changes in the parser and the AlterViewStmt constructor. Here's an example syntax: alter view foo (a, b comment 'helloworld') as select * from bar; describe foo; +--+++ | name | type | comment| +--+++ | a| string || | b| string | helloworld | +--+++ The following tests were modified: 1. ParserTest - To check that the parser handles column definitions for alter view statements. 2. AnalyzerDDLTest - To ensure that the analyzer supports the change column definitions parsed. 3. TestDdlStatements - To verify the end-to-end functioning of ALTER VIEW statements with change column definitions. 4. AuthorizationTest - To ensure that alter table commands with column definitions check permissions as expected. Change-Id: I6073444a814a24d97e80df15fcd39be2812f63fc --- M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M testdata/workloads/functional-query/queries/QueryTest/views-ddl.test 8 files changed, 148 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/10720/4 -- 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: newpatchset Gerrit-Change-Id: I6073444a814a24d97e80df15fcd39be2812f63fc Gerrit-Change-Number: 10720 Gerrit-PatchSet: 4 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
Fredy Wijaya 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:
(2 comments)
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@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.
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.alltypes_view (cnt) as "
Can we alter a view from its own?
alter view functional.alltypes_view (new_int_col) as select int_col from
functional.alltypes_view
If yes, does that mean that this is one possible way to rename column names and
comments?
--
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
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Pooja Nilangekar
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 14 Jun 2018 23:25:02 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
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 columnDefs,
QueryStmt viewDefStmt) {
> Use List instead of ArrayList
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
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Pooja Nilangekar
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 14 Jun 2018 23:12:49 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
Fredy Wijaya 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:
(12 comments)
Can also update the AuthorizationTest with the new syntax?
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.
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 columnDefs,
QueryStmt viewDefStmt) {
Use List instead of ArrayList
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
line. Similarly with the rest of the code below.
http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1146
PS2, Line 1146:
nit: remove indentation
http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1148
PS2, Line 1148:
nit: remove indentation
http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1150
PS2, Line 1150:
nit: remove indentation
http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1152
PS2, Line 1152:
nit: remove indentation
http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1155
PS2, Line 1155:
nit: remove indentation
http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1156
PS2, Line 1156:
nit: remove indentation
http://gerrit.cloudera.org:8080/#/c/10720/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1160
PS2, Line 1160:
nit: remove indentation
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@241
PS2, Line 241: 'View has been altered.'
Can we also verify the altered view?
http://gerrit.cloudera.org:8080/#/c/10720/2/testdata/workloads/functional-query/queries/QueryTest/views-ddl.test@248
PS2, Line 248: 'View has been altered.'
Verify the altered view.
--
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
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Pooja Nilangekar
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Thu, 14 Jun 2018 22:28:59 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6305: Allow column definitions in ALTER VIEW
Pooja Nilangekar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10720 Change subject: IMPALA-6305: Allow column definitions in ALTER VIEW .. IMPALA-6305: Allow column definitions in ALTER VIEW This change adds support to change column definitions in ALTER VIEW statements. This support only required minor changes in the parser and the AlterViewStmt constructor. The following tests were modified: 1. ParserTest - To check that the parser handles column definitions for alter view statements. 2. AnalyzerDDLTest - To ensure that the analyzer supports the change column definitions parsed. 3. TestDdlStatements - To verify the end-to-end functioning of ALTER VIEW statements with change column definitions. Change-Id: I6073444a814a24d97e80df15fcd39be2812f63fc --- M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M testdata/workloads/functional-query/queries/QueryTest/views-ddl.test 5 files changed, 74 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/10720/2 -- 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: newchange Gerrit-Change-Id: I6073444a814a24d97e80df15fcd39be2812f63fc Gerrit-Change-Number: 10720 Gerrit-PatchSet: 2 Gerrit-Owner: Pooja Nilangekar Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong
