You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org> on 2018/07/10 22:16:32 UTC
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Pooja Nilangekar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10908
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Previously, ALTER VIEW queries did not carry out reference checks
in the analysis phase. This allowed the DDL operation to succeed
but subsequent queries to the view threw StackOverflowError
because the catalog was unable to resolve the reference. With this
change, the AlterViewStmt checks for direct and in-direct self
references before altering a view.
Testing: Added tests to AnalyzeDDLTest to verify it.
Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
---
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
2 files changed, 59 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/10908/2
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 2
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 14: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 14
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 22:14:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10908/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/10908/2/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@65
PS2, Line 65: TableRef tblRef = new TableRef(tableName_.toPath(), null);
: tblRef = analyzer.resolveTableRef(tblRef);
nit: can be simplified to TableRef tblRef = analyzer.resolveTableRef(new TableRef(tableName_.toPath(), null));
http://gerrit.cloudera.org:8080/#/c/10908/2/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@79
PS2, Line 79: Self reference
nit: Self-reference
http://gerrit.cloudera.org:8080/#/c/10908/2/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@91
PS2, Line 91: throw new AnalysisException("Subqueries not supported for " +
: stmt.getClass().getSimpleName() + " statements");
I'm a bit torn whether we should throw an AnalysisException or have a Preconditions check since this condition should never happen given that the parser doesn't support this syntax. Let me know what you think?
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 2
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Jul 2018 18:36:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 13: Code-Review+2
Carrying over Bharath's +2
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 13
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 17:49:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 11:
(1 comment)
Can +2 once you fix this remaining bug.
http://gerrit.cloudera.org:8080/#/c/10908/11/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
File fe/src/main/java/org/apache/impala/analysis/UnionStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/11/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java@557
PS11, Line 557: for (UnionOperand operand : operands_) {
super.collectInlineViews(....)
Add a test for this too. (view self ref from withClause_ in a UnionStmt).
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 11
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 01:21:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/27/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 10
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jul 2018 01:51:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10908/11/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
File fe/src/main/java/org/apache/impala/analysis/UnionStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/11/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java@557
PS11, Line 557: super.collectInlineViews(inlineViews);
> super.collectInlineViews(....)
Done
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 12
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 17:03:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10908/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/10908/2/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@91
PS2, Line 91: stmt.getClass().getSimpleName() + " statements");
: }
> Personally, I think the exception make sense when some external event cause
Thanks for the explanation!
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 4
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Jul 2018 19:02:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10908/4/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/10908/4/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@75
PS4, Line 75: InlineViewRef fromViewRef = (InlineViewRef) fromTblRef;
> That was my initial approach. However, I faced the issue that while getting
right, we ideally need to implement something like collectInlineViewRefs() in all the subclasses but that is probably not worth the effort since it is not used anywhere else.
http://gerrit.cloudera.org:8080/#/c/10908/6/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/10908/6/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@69
PS6, Line 69: while (!subQueries.isEmpty()) {
: QueryStmt stmt = subQueries.pop();
: if (stmt instanceof SelectStmt) {
: List<TableRef> fromTblRefs = ((SelectStmt) stmt).getTableRefs();
: for (TableRef fromTblRef : fromTblRefs) {
: if (fromTblRef instanceof InlineViewRef) {
: InlineViewRef fromViewRef = (InlineViewRef) fromTblRef;
: if (fromViewRef.getView() == ((InlineViewRef) tblRef).getView()) {
: throw new AnalysisException(String.format(
: "Self-reference not allowed on view: %s", tblRef.toSql()));
: } else {
: subQueries.push(fromViewRef.getViewStmt());
: }
: }
: }
: QueryStmt whereStmt = ((SelectStmt) stmt).getWhereQueryStmt();
: if (whereStmt != null) {
: subQueries.add(whereStmt);
: }
: } else if (stmt instanceof UnionStmt) {
: List<UnionOperand> unionOperands = ((UnionStmt) stmt).getOperands();
: for (UnionOperand operand : unionOperands) {
: subQueries.push(operand.getQueryStmt());
: }
: } else {
: throw new AnalysisException("Subqueries not supported for " +
: stmt.getClass().getSimpleName() + " statements");
: }
:
How about factoring this out into a method in QueryStmt and have helper methods in SelectStmt and UnionStmt to collect each of their own view refs? That way you could do something like if (viewDefStmt_.collectInlineViewRefs().contains(tblRef.getView())...
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Jul 2018 17:28:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#13). ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Previously, ALTER VIEW queries did not carry out reference checks
in the analysis phase. This allowed the DDL operation to succeed
but subsequent queries to the view threw StackOverflowError
because the catalog was unable to resolve the reference. With this
change, the AlterViewStmt checks for direct and in-direct self
references before altering a view.
Testing: Added tests to AnalyzeDDLTest to verify it.
Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
---
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
5 files changed, 123 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/10908/13
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 13
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 10:
(3 comments)
Sorry for the delay, I was busy this week.
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@138
PS7, Line 138: FeView
> I tried the changes discussed offline. However, the InlineViewRef objects a
Makes sense. So can we call this collectIlineViews(..views) or something since we are not collecting ViewRefs anyway?
http://gerrit.cloudera.org:8080/#/c/10908/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@138
PS8, Line 138: public abstract void collectInlineViewRefs(Set<FeView> inlineViewRefs);
> Done.
Let's keep both the versions for completeness. Looks like you replaced the earlier one.
http://gerrit.cloudera.org:8080/#/c/10908/10/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/10/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@1092
PS10, Line 1092: if (withClause_ != null) {
: List<? extends FeView> withClauseViews = withClause_.getViews();
: for (FeView withView : withClauseViews) {
: inlineViewRefs.add(withView);
: withView.getQueryStmt().collectInlineViewRefs(inlineViewRefs);
: }
: }
Should this be moved to the QueryStmt class? See implementation of collectTableRefs() for example.
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 10
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Sat, 28 Jul 2018 00:06:45 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 11:
Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/95/
Running initial code review checks. This is experimental - please report any issues to tarmstrong@cloudera.com or on this JIRA: IMPALA-7317
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 11
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Sat, 28 Jul 2018 00:21:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 14:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2880/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 14
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 22:50:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 10:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@138
PS7, Line 138: FeView
> As discussed offline, using InlineViewRef makes it more consistent with col
I tried the changes discussed offline. However, the InlineViewRef objects are not equal (their corresponding SQL strings match but the hash of the objects are different). On the other hand, equality check of FeView only compares the QueryStmt and the ColRefs which are equal.
http://gerrit.cloudera.org:8080/#/c/10908/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@138
PS8, Line 138: public abstract void collectInlineViewRefs(Set<FeView> inlineViewRefs);
> Shouldn't we also collect view refs from withClause_? I see you added a tes
Done.
I have also changed the test.
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@148
PS7, Line 148: getWhereSubQuery
> I think is is ok since it conveys the intention better than getWhereQuerySt
Done
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 10
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jul 2018 01:14:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 13:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/104/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 13
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 22:37:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 14:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2878/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 14
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 22:14:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#11). ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Previously, ALTER VIEW queries did not carry out reference checks
in the analysis phase. This allowed the DDL operation to succeed
but subsequent queries to the view threw StackOverflowError
because the catalog was unable to resolve the reference. With this
change, the AlterViewStmt checks for direct and in-direct self
references before altering a view.
Testing: Added tests to AnalyzeDDLTest to verify it.
Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
---
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
5 files changed, 108 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/10908/11
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 11
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 12: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10908/12/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/12/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@147
PS12, Line 147: // Returns the QueryStmt present in the whereClause_ if present, null otherwise.
nit: Use /* */ style comments for methods
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 12
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 17:20:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 7:
(7 comments)
Looks pretty close.
http://gerrit.cloudera.org:8080/#/c/10908/7/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/10908/7/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@21
PS7, Line 21: import java.util.LinkedList;
Remove
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@24
PS7, Line 24: import org.apache.impala.analysis.UnionStmt.UnionOperand;
Remove
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@138
PS7, Line 138: FeView
Just curious, why can't we collect Set<InlineViewRef> directly?
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@147
PS7, Line 147: // Returns the QueryStmt present in the whereClause_ of the SelectStmt.
...Null otherwise...
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@148
PS7, Line 148: getWhereQueryStmt
I think getWhereSubQueryStmt() makes more sense. Also make it private?
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@1076
PS7, Line 1076: public void collectInlineViewRefs(Set<FeView> inlineViewRefs) {
nit: Mention that currently only from/where subqueries are supported. Other clauses like having.... etc are not allowed.
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@1077
PS7, Line 1077: List<TableRef> fromTblRefs = getTableRefs();
Preconditions.checkNotNull(inlineViewRefs);
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 7
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 06:45:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 12:
Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/99/
Running initial code review checks. This is experimental - please report any issues to tarmstrong@cloudera.com or on this JIRA: IMPALA-7317
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 12
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 17:03:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 7:
(1 comment)
I am sorry I had to rebase this change. I couldn't get impalad to start due to IMPALA-7316.
http://gerrit.cloudera.org:8080/#/c/10908/6/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/10908/6/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@69
PS6, Line 69: if (inlineViewRefs.contains(((InlineViewRef) tblRef).getView())) {
: throw new AnalysisException(
: String.format("Self-reference not allowed on view: %s", tblRef.toSql()));
: }
:
: createColumnAndViewDefs(analyzer);
: if (BackendConfig.INSTANCE.getComputeLineage() || RuntimeEnv.INSTANCE.isTestEnv()) {
: computeLineageGraph(analyzer);
: }
: }
:
: @Override
: public String toSql() {
: StringBuilder sb = new StringBuilder();
: sb.append("ALTER VIEW ");
: if (tableName_.getDb() != null) {
: sb.append(tableName_.getDb() + ".");
: }
: sb.append(tableName_.getTbl());
: if (columnDefs_ != null) sb.append("(" + getColumnNames() + ")");
: sb.append(" AS " + viewDefStmt_.toSql());
: return sb.toString();
: }
: }
:
:
:
:
:
> How about factoring this out into a method in QueryStmt and have helper met
Done
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 7
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Sat, 21 Jul 2018 01:30:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10908/12/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/12/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@147
PS12, Line 147: /**
> nit: Use /* */ style comments for methods
Done
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 13
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 17:43:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#12). ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Previously, ALTER VIEW queries did not carry out reference checks
in the analysis phase. This allowed the DDL operation to succeed
but subsequent queries to the view threw StackOverflowError
because the catalog was unable to resolve the reference. With this
change, the AlterViewStmt checks for direct and in-direct self
references before altering a view.
Testing: Added tests to AnalyzeDDLTest to verify it.
Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
---
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
5 files changed, 121 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/10908/12
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 12
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Previously, ALTER VIEW queries did not carry out reference checks
in the analysis phase. This allowed the DDL operation to succeed
but subsequent queries to the view threw StackOverflowError
because the catalog was unable to resolve the reference. With this
change, the AlterViewStmt checks for direct and in-direct self
references before altering a view.
Testing: Added tests to AnalyzeDDLTest to verify it.
Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
---
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
5 files changed, 93 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/10908/7
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 7
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 10:
Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/27/
Running initial code review checks. This is experimental - please report any issues to tarmstrong@cloudera.com or on this JIRA: IMPALA-7317
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 10
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Jul 2018 01:14:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Previously, ALTER VIEW queries did not carry out reference checks
in the analysis phase. This allowed the DDL operation to succeed
but subsequent queries to the view threw StackOverflowError
because the catalog was unable to resolve the reference. With this
change, the AlterViewStmt checks for direct and in-direct self
references before altering a view.
Testing: Added tests to AnalyzeDDLTest to verify it.
Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Reviewed-on: http://gerrit.cloudera.org:8080/10908
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
5 files changed, 123 insertions(+), 1 deletion(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 15
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 13:
Build Started https://jenkins.impala.io/job/gerrit-code-review-checks/104/
Running initial code review checks. This is experimental - please report any issues to tarmstrong@cloudera.com or on this JIRA: IMPALA-7317
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 13
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 17:46:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 12:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/99/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 12
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Jul 2018 17:37:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Previously, ALTER VIEW queries did not carry out reference checks
in the analysis phase. This allowed the DDL operation to succeed
but subsequent queries to the view threw StackOverflowError
because the catalog was unable to resolve the reference. With this
change, the AlterViewStmt checks for direct and in-direct self
references before altering a view.
Testing: Added tests to AnalyzeDDLTest to verify it.
Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
---
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
2 files changed, 58 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/10908/4
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 4
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 8:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/10908/7/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/10908/7/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@21
PS7, Line 21: import java.util.Set;
> Remove
Done
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@24
PS7, Line 24: import org.apache.impala.catalog.FeTable;
> Remove
Done
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@138
PS7, Line 138: FeView
> Just curious, why can't we collect Set<InlineViewRef> directly?
I had no semantic reason, I was only trying to follow the convention of collectTableRefs. In that function the TableRefs are collected in SQL clause order.
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@147
PS7, Line 147: // Returns the QueryStmt present in the whereClause_ if present, null otherwise.
> ...Null otherwise...
Done
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@148
PS7, Line 148: getWhereQueryStm
> I think getWhereSubQueryStmt() makes more sense. Also make it private?
I have changed it to private. I initially named it getWhereSubQueryStmt but then noticed that there is another class called SubQuery. So I thought it might be misleading. Do you think it would be okay to change it?
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@1076
PS7, Line 1076: public void collectInlineViewRefs(Set<FeView> inlineViewRefs) {
> nit: Mention that currently only from/where subqueries are supported. Other
Done
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@1077
PS7, Line 1077: // Impala currently supports sub queries only in FROM & WHERE clauses. Hence, this
> Preconditions.checkNotNull(inlineViewRefs);
Done
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 8
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 18:07:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Previously, ALTER VIEW queries did not carry out reference checks
in the analysis phase. This allowed the DDL operation to succeed
but subsequent queries to the view threw StackOverflowError
because the catalog was unable to resolve the reference. With this
change, the AlterViewStmt checks for direct and in-direct self
references before altering a view.
Testing: Added tests to AnalyzeDDLTest to verify it.
Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
---
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
5 files changed, 101 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/10908/10
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 10
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@138
PS7, Line 138: FeView
> I had no semantic reason, I was only trying to follow the convention of col
As discussed offline, using InlineViewRef makes it more consistent with collectTableRefs()
http://gerrit.cloudera.org:8080/#/c/10908/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/8/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@138
PS8, Line 138: public abstract void collectInlineViewRefs(Set<FeView> inlineViewRefs);
Shouldn't we also collect view refs from withClause_? I see you added a test for it but something like the following would fail. Basically with clause's subqueries not referenced in from clause of the select stmt. (I couldn't think of a better example :D).
alter view v2 as with temp(col_a) as (select * from v2) select * from t2;
The reason the test case you added works is because the view referenced is also indirectly referenced in the fromClause_ of the SelectStmt.
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
http://gerrit.cloudera.org:8080/#/c/10908/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@148
PS7, Line 148: getWhereQueryStm
> I have changed it to private. I initially named it getWhereSubQueryStmt but
I think is is ok since it conveys the intention better than getWhereQueryStmt().
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 8
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 23:57:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Previously, ALTER VIEW queries did not carry out reference checks
in the analysis phase. This allowed the DDL operation to succeed
but subsequent queries to the view threw StackOverflowError
because the catalog was unable to resolve the reference. With this
change, the AlterViewStmt checks for direct and in-direct self
references before altering a view.
Testing: Added tests to AnalyzeDDLTest to verify it.
Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
---
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
3 files changed, 85 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/10908/6
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10908/4/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/10908/4/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@72
PS4, Line 72: ((SelectStmt) stmt).getTableRefs();
> This only gets you fromClause_'s TableRefs. For example something like
Done.
I have added test cases for where clause and with clause. Other classes like "GROUP BY", "ORDER BY" and "HAVING" don't support sub-queries.
http://gerrit.cloudera.org:8080/#/c/10908/4/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@75
PS4, Line 75: InlineViewRef fromViewRef = (InlineViewRef) fromTblRef;
> While I see the intention here, a cleaner way for this is to do something l
That was my initial approach. However, I faced the issue that while getting the TableRefs from the viewDefStmt_, the function resolves it to the underlying table definition. Since at this point, the view definition is still not altered. Yet, the SQL statement contains references to the view itself. So this approach ensures that we get all the view references that need to be resolved instead of getting just the eventual base table references.
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 6
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Jul 2018 20:56:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Pooja Nilangekar (Code Review)" <ge...@cloudera.org>.
Pooja Nilangekar has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Previously, ALTER VIEW queries did not carry out reference checks
in the analysis phase. This allowed the DDL operation to succeed
but subsequent queries to the view threw StackOverflowError
because the catalog was unable to resolve the reference. With this
change, the AlterViewStmt checks for direct and in-direct self
references before altering a view.
Testing: Added tests to AnalyzeDDLTest to verify it.
Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
---
M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
5 files changed, 94 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/10908/8
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 8
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10908/4/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/10908/4/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@72
PS4, Line 72: ((SelectStmt) stmt).getTableRefs();
This only gets you fromClause_'s TableRefs. For example something like
alter view v1 as select * from test1 where a > (select count(*) from v1);
would pass this and then the view analysis causes stack overflow.
(add tests for non-from clause cases)
http://gerrit.cloudera.org:8080/#/c/10908/4/fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java@75
PS4, Line 75: InlineViewRef fromViewRef = (InlineViewRef) fromTblRef;
While I see the intention here, a cleaner way for this is to do something like what QueryStmt#collectTableRefs() does. While the name says "TableRefs", it also expands the InlineViewRefs to populate the actual base TableRefs.
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 4
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Sat, 14 Jul 2018 22:42:02 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/95/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 11
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Sat, 28 Jul 2018 01:01:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 4:
Bharath, can you review this for the +2?
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 4
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Sat, 14 Jul 2018 01:05:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7209: Disallow self referencing in ALTER VIEW statements
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10908 )
Change subject: IMPALA-7209: Disallow self referencing in ALTER VIEW statements
......................................................................
Patch Set 14: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17c231c9d74d9d411463a408b086eb874090b9b7
Gerrit-Change-Number: 10908
Gerrit-PatchSet: 14
Gerrit-Owner: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Pooja Nilangekar <po...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Jul 2018 02:11:51 +0000
Gerrit-HasComments: No