[Impala-ASF-CR] IMPALA-6605: Exception hidden on complex types
Fredy Wijaya has abandoned this change. ( http://gerrit.cloudera.org:8080/9514 ) Change subject: IMPALA-6605: Exception hidden on complex types .. Abandoned Abandon for now. -- To view, visit http://gerrit.cloudera.org:8080/9514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I3ae62fe63b82899d1467a23422d4a73f1471aa84 Gerrit-Change-Number: 9514 Gerrit-PatchSet: 12 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya
[Impala-ASF-CR] IMPALA-6605: Exception hidden on complex types
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9514 ) Change subject: IMPALA-6605: Exception hidden on complex types .. Patch Set 12: Fredy and I discussed this change and we concluded that we are not convinced that it works as intended in all cases, or whether it introduces unintended consequences or inconsistency in behavior. This code is very tricky and we need to tread carefully. To move forward with this change, I think we need to first add a auth test suite that systematically enumerates the cases and tests their expected behavior. Once we have a complete picture of the challenging cases, we can talk about modifying this code. My gut feeling is that some internal interfaces are not quite right for serving this new desired behavior, and making such a drastic change only makes sense when all cases are well understood and tested. The different combinations of cases in my mind (there might be more): * All path types: table ref, slot ref, star * Absolute table/slot/star paths * Relative table/slot/star paths (rooted a registered alias) * Failure cases: 1. path has no resolution 2. path has one or more resolutions, but they are not legal for the given path type 3. path has multiple legal resolutions (path is ambiguous) -- To view, visit http://gerrit.cloudera.org:8080/9514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3ae62fe63b82899d1467a23422d4a73f1471aa84 Gerrit-Change-Number: 9514 Gerrit-PatchSet: 12 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya Gerrit-Comment-Date: Fri, 09 Mar 2018 17:22:30 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6605: Exception hidden on complex types
Fredy Wijaya has uploaded a new patch set (#12). ( http://gerrit.cloudera.org:8080/9514 ) Change subject: IMPALA-6605: Exception hidden on complex types .. IMPALA-6605: Exception hidden on complex types This patch fixes the issue when we have a column privilege on a particular table, but the statement has an analysis error. Example: > grant select(int_struct_col) on table functional.allcomplextypes > to role test_role; > select 1 from functional.allcomplextypes.int_struct_col; The select statement above should throw an AnalysisException and not an AuthorizationException since we have int_struct_col column privilege on functional.allcomplextypes table. Testing: - Ran all front-end tests Change-Id: I3ae62fe63b82899d1467a23422d4a73f1471aa84 --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java A fe/src/main/java/org/apache/impala/common/PathResolutionException.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java 3 files changed, 115 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/9514/12 -- To view, visit http://gerrit.cloudera.org:8080/9514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3ae62fe63b82899d1467a23422d4a73f1471aa84 Gerrit-Change-Number: 9514 Gerrit-PatchSet: 12 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya
[Impala-ASF-CR] IMPALA-6605: Exception hidden on complex types
Fredy Wijaya has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/9514 ) Change subject: IMPALA-6605: Exception hidden on complex types .. IMPALA-6605: Exception hidden on complex types This patch fixes the issue when we have a column privilege on a particular table, but the statement has an analysis error. Example: > grant select(int_struct_col) on table functional.allcomplextypes > to role test_role; > select 1 from functional.allcomplextypes.int_struct_col; The select statement above should throw an AnalysisException and not an AuthorizationException since we have int_struct_col column privilege on functional.allcomplextypes table. Testing: - Ran all front-end tests Change-Id: I3ae62fe63b82899d1467a23422d4a73f1471aa84 --- M fe/src/main/java/org/apache/impala/analysis/Analyzer.java A fe/src/main/java/org/apache/impala/common/PathResolutionException.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java 3 files changed, 105 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/9514/9 -- To view, visit http://gerrit.cloudera.org:8080/9514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3ae62fe63b82899d1467a23422d4a73f1471aa84 Gerrit-Change-Number: 9514 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya
[Impala-ASF-CR] IMPALA-6605: Exception hidden on complex types
Alex Behm has posted comments on this change. (
http://gerrit.cloudera.org:8080/9514 )
Change subject: IMPALA-6605: Exception hidden on complex types
..
Patch Set 5:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/9514/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:
http://gerrit.cloudera.org:8080/#/c/9514/5/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@583
PS5, Line 583: boolean hasAuthorizeableTable = false;
To simplify things, could we not also add an ANY Table-level priv request? It
seems like now we can have a "naked" column priv request in this codepath. This
change breaks comments and assumptions made elsewhere. Specifically, look at:
AnalysisContext.java L484 and the comment above it:
// Authorize statements that may produce several hierarchical privilege
requests.
// Such a statement always has a corresponding table-level privilege
request if it
// has column-level privilege request. The hierarchical nature requires
special
// logic to process correctly and efficiently.
I think this change breaks that assertion, but it would be generally preferable
to maintain it.
http://gerrit.cloudera.org:8080/#/c/9514/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
http://gerrit.cloudera.org:8080/#/c/9514/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@566
PS5, Line 566: } catch (AnalysisException e) {
There are 3 distinct causes for getting an exception here:
1. path has no resolution
2. path has one or more resolutions, but they are not legal for the given path
type
3. path has multiple legal resolutions (path is ambiguous)
These changes deal with (1) and (2), but not (3).
For the 3rd case I think we can only return an AnalysisException to the user if
he has the right to access all the legal path interpretations. Otherwise, we
should return an AuthException.
http://gerrit.cloudera.org:8080/#/c/9514/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@567
PS5, Line 567: // Register privilege requests to prefer reporting an
authorization error over
Please move into a helper function.
http://gerrit.cloudera.org:8080/#/c/9514/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@585
PS5, Line 585: // authorization using real path that exists.
a real path
http://gerrit.cloudera.org:8080/#/c/9514/5/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@798
PS5, Line 798: tbl = getTable(tblName.getDb(), tblName.getTbl());
While you are here, can you fix this to use
globalState_.stmtTableCache.tables.get(tblName);
and check for a null?
No point in generating an exception in getTable() just to catch it here and do
nothing.
http://gerrit.cloudera.org:8080/#/c/9514/5/fe/src/main/java/org/apache/impala/common/AnalysisException.java
File fe/src/main/java/org/apache/impala/common/AnalysisException.java:
http://gerrit.cloudera.org:8080/#/c/9514/5/fe/src/main/java/org/apache/impala/common/AnalysisException.java@27
PS5, Line 27: private Path candidatePath_;
I don't think this belongs here. An AnalysisException is a very general type of
exception and it's not clear why a change to this fundamental exception is
warranted to fix this very narrow bug.
I can imagine several cleaner alternatives:
* Add a new PathResolutionException subclass and add to it the list of errors
and candidate paths
* Add a new ErrorPath subclass of Path and add candidates and errors to it.
Don't throw in resolvePath() and let callers decide how to deal with an
ErrorPath.
I think the first route is more expedient, but we may want to at least consider
the second alternative.
Thoughts?
http://gerrit.cloudera.org:8080/#/c/9514/5/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/9514/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@2219
PS5, Line 2219: private void AuthzOkAnalysisError(String stmt, String
expectedErrorString)
Isn't this just the existing AnalysisError() passing in analysisContext_?
--
To view, visit http://gerrit.cloudera.org:8080/9514
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3ae62fe63b82899d1467a23422d4a73f1471aa84
Gerrit-Change-Number: 9514
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Comment-Date: Thu, 08 Mar 2018 21:58:30 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6605: Exception hidden on complex types
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9514 ) Change subject: IMPALA-6605: Exception hidden on complex types .. Patch Set 5: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/9514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3ae62fe63b82899d1467a23422d4a73f1471aa84 Gerrit-Change-Number: 9514 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Comment-Date: Wed, 07 Mar 2018 22:00:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6605: Exception hidden on complex types
Fredy Wijaya has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/9514 ) Change subject: IMPALA-6605: Exception hidden on complex types .. IMPALA-6605: Exception hidden on complex types This patch fixes the issue when we have a column privilege on a particular table, but the statement has an analysis error. Example: > grant select(int_struct_col) on table functional.allcomplextypes > to role test_role; > select 1 from functional.allcomplextypes.int_struct_col; The select statement above should throw an AnalysisException and AuthorizationException since we have int_struct_col column ege on functional.allcomplextypes table. Testing: - Ran all front-end tests Change-Id: I3ae62fe63b82899d1467a23422d4a73f1471aa84 --- M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/common/AnalysisException.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java 4 files changed, 79 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/9514/5 -- To view, visit http://gerrit.cloudera.org:8080/9514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3ae62fe63b82899d1467a23422d4a73f1471aa84 Gerrit-Change-Number: 9514 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya
[Impala-ASF-CR] IMPALA-6605: Exception hidden on complex types
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9514 ) Change subject: IMPALA-6605: Exception hidden on complex types .. Patch Set 4: (2 comments) Couple nits. http://gerrit.cloudera.org:8080/#/c/9514/4/fe/src/main/java/org/apache/impala/common/AnalysisException.java File fe/src/main/java/org/apache/impala/common/AnalysisException.java: http://gerrit.cloudera.org:8080/#/c/9514/4/fe/src/main/java/org/apache/impala/common/AnalysisException.java@27 PS4, Line 27: candidatePath Private variables end in "_". http://gerrit.cloudera.org:8080/#/c/9514/4/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/9514/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@576 PS4, Line 576: "'functional.allcomplextypes.int_struct_col'") Fix indent. -- To view, visit http://gerrit.cloudera.org:8080/9514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3ae62fe63b82899d1467a23422d4a73f1471aa84 Gerrit-Change-Number: 9514 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Comment-Date: Wed, 07 Mar 2018 16:31:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6605: Exception hidden on complex types
Fredy Wijaya has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/9514 ) Change subject: IMPALA-6605: Exception hidden on complex types .. IMPALA-6605: Exception hidden on complex types This patch fixes the issue when we have a column privilege on a particular table, but the statement has an analysis error. Example: > grant select(int_struct_col) on table functional.allcomplextypes > to role test_role; > select 1 from functional.allcomplextypes.int_struct_col; The select statement above should throw an AnalysisException and AuthorizationException since we have int_struct_col column ege on functional.allcomplextypes table. Testing: - Ran all front-end tests Change-Id: I3ae62fe63b82899d1467a23422d4a73f1471aa84 --- M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/common/AnalysisException.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java 4 files changed, 79 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/9514/4 -- To view, visit http://gerrit.cloudera.org:8080/9514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3ae62fe63b82899d1467a23422d4a73f1471aa84 Gerrit-Change-Number: 9514 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya
[Impala-ASF-CR] IMPALA-6605: Exception hidden on complex types
Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9514 Change subject: IMPALA-6605: Exception hidden on complex types .. IMPALA-6605: Exception hidden on complex types This patch fixes the issue when we have a column privilege on a particular table, but the statement has an analysis error. Example: > grant select(int_struct_col) on table functional.allcomplextypes > to role test_role; > select 1 from functional.allcomplextypes.int_struct_col; The select statement above should throw an AnalysisException and not an AuthorizationException since we have int_struct_col column privilege on functional.allcomplextypes table. Testing: - Ran all front-end tests Change-Id: I3ae62fe63b82899d1467a23422d4a73f1471aa84 --- M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/common/AnalysisException.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java 4 files changed, 78 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/9514/3 -- To view, visit http://gerrit.cloudera.org:8080/9514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I3ae62fe63b82899d1467a23422d4a73f1471aa84 Gerrit-Change-Number: 9514 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya
