[Impala-ASF-CR] IMPALA-6573: Create consistent response on column access failures

2018-03-15 Thread Adam Holley (Code Review) 
Adam Holley has abandoned this change. ( http://gerrit.cloudera.org:8080/9509 )

Change subject: IMPALA-6573: Create consistent response on column access 
failures
..


Abandoned

Duplicate of 9514.
--
To view, visit http://gerrit.cloudera.org:8080/9509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I9676a21a171862bb1f664e82d0a7d1db0d36462a
Gerrit-Change-Number: 9509
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya

[Impala-ASF-CR] IMPALA-6573: Create consistent response on column access failures

2018-03-14 Thread Alex Behm (Code Review) 
Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9509 )

Change subject: IMPALA-6573: Create consistent response on column access 
failures
..


Patch Set 2:

This is the same problem as IMPALA-6605 which has a code review here:
https://gerrit.cloudera.org/#/c/9514/

I outlined what we need to do to efficiently address these error 
inconsistencies for failed path resolutions in the above code review.

I suggest we abandon this CR and focus on 9514


--
To view, visit http://gerrit.cloudera.org:8080/9509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9676a21a171862bb1f664e82d0a7d1db0d36462a
Gerrit-Change-Number: 9509
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Wed, 14 Mar 2018 20:01:13 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6573: Create consistent response on column access failures

2018-03-09 Thread Alex Behm (Code Review) 
Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9509 )

Change subject: IMPALA-6573: Create consistent response on column access 
failures
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9509/2/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/9509/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2394
PS2, Line 2394:   .setColumnVisible(false).toRequest());
I don't think this is the right fix. Based on your tests the discrepancy in 
DESCRIBE is as follows:

describe functional.complextypes_fileformat.id
Reports: .*

versus
describe functional.complextypes_fileformat.zz
Reports: 

Imo the bug is in DescribeTableStmt.java in L121 in the handling of the 
AnalysisException returned from path.resolve().

The right fix is to change the handling of rawPath_.size() > 1 to the following 
slightly different snippet:

  if (rawPath_.size() > 1) {
analyzer.registerPrivReq(new PrivilegeRequestBuilder()
.any().onAnyColumn(rawPath_.get(0), rawPath_.get(1)).toRequest());
  }

After that change both queries above consistently report .*


http://gerrit.cloudera.org:8080/#/c/9509/2/fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java:

http://gerrit.cloudera.org:8080/#/c/9509/2/fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java@63
PS2, Line 63: if (isColumnVisible()) {
For changes like this we need to be really convinced that we are fixing the 
right thing. Changing the output of getName() for a AuthorizeableColumn seems 
really odd. Why would the name of the column change depending on some flag? 
Semantics seem weird to me.

Anyway, see my other comment on why I think this isn't even the right fix.


http://gerrit.cloudera.org:8080/#/c/9509/2/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/9509/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@574
PS2, Line 574: AuthzError("select 1 from 
functional.complex_nested_struct_col.f1",
As far as I can tell, these tests also worked as expected before the fix (I 
tried on my machine). Did you just add the for completeness?


http://gerrit.cloudera.org:8080/#/c/9509/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1489
PS2, Line 1489: AuthzError("describe functional.complextypes_fileformat.id",
These tests also pass without your fix because we prefix match the expected 
error message, so they don't really cover this bug.



--
To view, visit http://gerrit.cloudera.org:8080/9509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9676a21a171862bb1f664e82d0a7d1db0d36462a
Gerrit-Change-Number: 9509
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Fri, 09 Mar 2018 19:34:48 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6573: Create consistent response on column access failures

2018-03-07 Thread Fredy Wijaya (Code Review) 
Fredy Wijaya has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9509 )

Change subject: IMPALA-6573: Create consistent response on column access 
failures
..


Patch Set 2: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9509/2/fe/src/main/java/org/apache/impala/authorization/Authorizeable.java
File fe/src/main/java/org/apache/impala/authorization/Authorizeable.java:

http://gerrit.cloudera.org:8080/#/c/9509/2/fe/src/main/java/org/apache/impala/authorization/Authorizeable.java@56
PS2, Line 56: }
> I wonder what was changed here. If it's unintentional, can you revert the c
There seems to be something weird with Gerrit. I can't see this change when I 
pulled your review.



--
To view, visit http://gerrit.cloudera.org:8080/9509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9676a21a171862bb1f664e82d0a7d1db0d36462a
Gerrit-Change-Number: 9509
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Thu, 08 Mar 2018 04:05:57 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6573: Create consistent response on column access failures

2018-03-07 Thread Fredy Wijaya (Code Review) 
Fredy Wijaya has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9509 )

Change subject: IMPALA-6573: Create consistent response on column access 
failures
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9509/2/fe/src/main/java/org/apache/impala/authorization/Authorizeable.java
File fe/src/main/java/org/apache/impala/authorization/Authorizeable.java:

http://gerrit.cloudera.org:8080/#/c/9509/2/fe/src/main/java/org/apache/impala/authorization/Authorizeable.java@56
PS2, Line 56: }
I wonder what was changed here. If it's unintentional, can you revert the 
changes to this file?



--
To view, visit http://gerrit.cloudera.org:8080/9509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9676a21a171862bb1f664e82d0a7d1db0d36462a
Gerrit-Change-Number: 9509
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Thu, 08 Mar 2018 02:37:12 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6573: Create consistent response on column access failures

2018-03-07 Thread Adam Holley (Code Review) 
Adam Holley has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/9509 )

Change subject: IMPALA-6573: Create consistent response on column access 
failures
..

IMPALA-6573: Create consistent response on column access failures

This fixes a bug where failure messages were inconsistent
depending on whether valid or invalid objects were included in the
statement.

Testing:

Created validation tests for both select and describe.

Change-Id: I9676a21a171862bb1f664e82d0a7d1db0d36462a
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
5 files changed, 59 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/9509/2
--
To view, visit http://gerrit.cloudera.org:8080/9509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9676a21a171862bb1f664e82d0a7d1db0d36462a
Gerrit-Change-Number: 9509
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya

[Impala-ASF-CR] IMPALA-6573: Create consistent response on column access failures

2018-03-07 Thread Adam Holley (Code Review) 
Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9509 )

Change subject: IMPALA-6573: Create consistent response on column access 
failures
..


Patch Set 1:

(1 comment)

I've made changes to switch to column specific.  It is not reproducible with 
tables since the onAnyTable method is not used anywhere.

http://gerrit.cloudera.org:8080/#/c/9509/1/fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java:

http://gerrit.cloudera.org:8080/#/c/9509/1/fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java@62
PS1, Line 62: if (isLastNodeVisible()) {
> It seems like it should also apply to tables, but I've be unable so far to
I've switched it to be column specific.  Tables are handled differently.



--
To view, visit http://gerrit.cloudera.org:8080/9509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9676a21a171862bb1f664e82d0a7d1db0d36462a
Gerrit-Change-Number: 9509
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Wed, 07 Mar 2018 23:37:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6573: Create consistent response on column access failures

2018-03-07 Thread Adam Holley (Code Review) 
Adam Holley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9509 )

Change subject: IMPALA-6573: Create consistent response on column access 
failures
..


Patch Set 1:

(4 comments)

Fixed.

http://gerrit.cloudera.org:8080/#/c/9509/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9509/1//COMMIT_MSG@11
PS1, Line 11: statement.
> It's good to give some examples on where the inconsistencies are.
Copied the one from the Jira.


http://gerrit.cloudera.org:8080/#/c/9509/1/fe/src/main/java/org/apache/impala/authorization/Authorizeable.java
File fe/src/main/java/org/apache/impala/authorization/Authorizeable.java:

http://gerrit.cloudera.org:8080/#/c/9509/1/fe/src/main/java/org/apache/impala/authorization/Authorizeable.java@47
PS1, Line 47:   // Flag for controlling whether last node in the path is 
visible.
> Why is this not private?
Done


http://gerrit.cloudera.org:8080/#/c/9509/1/fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java:

http://gerrit.cloudera.org:8080/#/c/9509/1/fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java@52
PS1, Line 52:
> Remove extra new line.
Done


http://gerrit.cloudera.org:8080/#/c/9509/1/fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java@62
PS1, Line 62: if (isLastNodeVisible()) {
> By looking at this implementation, last node is always the column. Isn't it
It seems like it should also apply to tables, but I've be unable so far to find 
a specific reproducible scenario.



--
To view, visit http://gerrit.cloudera.org:8080/9509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9676a21a171862bb1f664e82d0a7d1db0d36462a
Gerrit-Change-Number: 9509
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Wed, 07 Mar 2018 16:55:26 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6573: Create consistent response on column access failures

2018-03-06 Thread Fredy Wijaya (Code Review) 
Fredy Wijaya has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9509 )

Change subject: IMPALA-6573: Create consistent response on column access 
failures
..


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9509/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9509/1//COMMIT_MSG@11
PS1, Line 11: statement.
It's good to give some examples on where the inconsistencies are.


http://gerrit.cloudera.org:8080/#/c/9509/1/fe/src/main/java/org/apache/impala/authorization/Authorizeable.java
File fe/src/main/java/org/apache/impala/authorization/Authorizeable.java:

http://gerrit.cloudera.org:8080/#/c/9509/1/fe/src/main/java/org/apache/impala/authorization/Authorizeable.java@47
PS1, Line 47:   // Flag for controlling whether last node in the path is 
visible.
Why is this not private?


http://gerrit.cloudera.org:8080/#/c/9509/1/fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java:

http://gerrit.cloudera.org:8080/#/c/9509/1/fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java@52
PS1, Line 52:
Remove extra new line.


http://gerrit.cloudera.org:8080/#/c/9509/1/fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java@62
PS1, Line 62: if (isLastNodeVisible()) {
By looking at this implementation, last node is always the column. Isn't it 
better to rename the method as isColumnVisible() instead?



--
To view, visit http://gerrit.cloudera.org:8080/9509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9676a21a171862bb1f664e82d0a7d1db0d36462a
Gerrit-Change-Number: 9509
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Tue, 06 Mar 2018 23:04:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6573: Create consistent response on column access failures

2018-03-06 Thread Adam Holley (Code Review) 
Adam Holley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9509


Change subject: IMPALA-6573: Create consistent response on column access 
failures
..

IMPALA-6573: Create consistent response on column access failures

This fixes a bug where failure messages were inconsistent
depending on whether valid or invalid objects were included in the
statement.

Testing:

Created validation tests for both select and describe.

Change-Id: I9676a21a171862bb1f664e82d0a7d1db0d36462a
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java
M fe/src/main/java/org/apache/impala/authorization/Authorizeable.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizeableColumn.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
6 files changed, 57 insertions(+), 6 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/9509/1
--
To view, visit http://gerrit.cloudera.org:8080/9509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9676a21a171862bb1f664e82d0a7d1db0d36462a
Gerrit-Change-Number: 9509
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley