You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Fredy Wijaya (Code Review)" <ge...@cloudera.org> on 2018/07/11 03:51:08 UTC
[Impala-ASF-CR] IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10918
Change subject: IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
......................................................................
IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
In order to do COMPUTE STATS, Impala performs several SELECT queries.
However, in the COMPUTE STATS analysis phase, we only register for the
ALTER privilege. Although the SELECT privilege is eventually registered
to the target table by the SELECT child queries, it is better to register
the SELECT privilege in the COMPUTE STATS analysis phase and fail early
if the privilege requirements are not met instead of failing later in the
SELCT child queries due to insufficient privileges.
Testing:
- Updated the authorization tests
- Ran all FE tests
Change-Id: Icead43e689404a286859344e309427eb6c68646a
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionDef.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
7 files changed, 89 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/10918/1
--
To view, visit http://gerrit.cloudera.org:8080/10918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icead43e689404a286859344e309427eb6c68646a
Gerrit-Change-Number: 10918
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
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/10918 )
Change subject: IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
......................................................................
IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
In order to do COMPUTE STATS, Impala performs several SELECT queries.
However, in the COMPUTE STATS analysis phase, we only check for the
ALTER privilege. Although the SELECT privilege is eventually checked
in the target table by the SELECT child queries, it is better to
check the SELECT privilege in the COMPUTE STATS analysis phase and
fail early if the privilege requirements are not met instead of failing
later in the SELECT child queries due to insufficient privileges.
Testing:
- Updated the authorization tests
- Ran all FE tests
Change-Id: Icead43e689404a286859344e309427eb6c68646a
Reviewed-on: http://gerrit.cloudera.org:8080/10918
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/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionDef.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
7 files changed, 93 insertions(+), 51 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/10918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Icead43e689404a286859344e309427eb6c68646a
Gerrit-Change-Number: 10918
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10918 )
Change subject: IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/10918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icead43e689404a286859344e309427eb6c68646a
Gerrit-Change-Number: 10918
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Jul 2018 21:15:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/10918 )
Change subject: IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
......................................................................
IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
In order to do COMPUTE STATS, Impala performs several SELECT queries.
However, in the COMPUTE STATS analysis phase, we only check for the
ALTER privilege. Although the SELECT privilege is eventually checked
in the target table by the SELECT child queries, it is better to
check the SELECT privilege in the COMPUTE STATS analysis phase and
fail early if the privilege requirements are not met instead of failing
later in the SELECT child queries due to insufficient privileges.
Testing:
- Updated the authorization tests
- Ran all FE tests
Change-Id: Icead43e689404a286859344e309427eb6c68646a
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionDef.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
7 files changed, 93 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/10918/4
--
To view, visit http://gerrit.cloudera.org:8080/10918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icead43e689404a286859344e309427eb6c68646a
Gerrit-Change-Number: 10918
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10918 )
Change subject: IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10918/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10918/1//COMMIT_MSG@15
PS1, Line 15: SELEC
> nit: SELECT
Done
http://gerrit.cloudera.org:8080/#/c/10918/1/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/10918/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2384
PS1, Line 2384: Always registers privilege request(s) for the table at the given privilege level(s),
: * regardless of the state of the table (i.e. whether it exists, is loaded, etc.).
: * If addAccessEvent is true adds access event(s) for successfully loaded tables.
> nit: Update comment to reflect multiple privileges and access events.
Done
http://gerrit.cloudera.org:8080/#/c/10918/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:
http://gerrit.cloudera.org:8080/#/c/10918/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@1185
PS1, Line 1185: nodb.notbl"), onDatabase("nodb", allExcept(
: TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER,
> missing TPrivilegeLevel.ALTER - alterError("nodb.notbl")
Done
http://gerrit.cloudera.org:8080/#/c/10918/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@1193
PS1, Line 1193: functional.notbl"), onDatabase("functional", allExcept(
: TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER,
> same as above - alterError("functional.notbl")
Done
http://gerrit.cloudera.org:8080/#/c/10918/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@1217
PS1, Line 1217: nodb.notbl"), onDatabase("nodb", allExcept(
: TPrivilegeLevel.ALL, TP
> same as above - alterError("nodb.notbl")
Done
http://gerrit.cloudera.org:8080/#/c/10918/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@1225
PS1, Line 1225: ("functional.notbl"), onDatabase("functional", allExcept(
: TPrivilegeLevel.ALL, TP
> same as above - alterError("functional.notbl")
Done
--
To view, visit http://gerrit.cloudera.org:8080/10918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icead43e689404a286859344e309427eb6c68646a
Gerrit-Change-Number: 10918
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Jul 2018 20:55:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10918 )
Change subject: IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10918/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:
http://gerrit.cloudera.org:8080/#/c/10918/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@1166
PS3, Line 1166: TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT
> There are test cases that are not applicable to "drop stats" if we don't un
that would be ALL + testPair.second
but ok, not too worried about this except that there is far more repetition than real difference.
--
To view, visit http://gerrit.cloudera.org:8080/10918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icead43e689404a286859344e309427eb6c68646a
Gerrit-Change-Number: 10918
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jul 2018 00:01:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10918 )
Change subject: IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icead43e689404a286859344e309427eb6c68646a
Gerrit-Change-Number: 10918
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jul 2018 03:33:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10918 )
Change subject: IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2830/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/10918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icead43e689404a286859344e309427eb6c68646a
Gerrit-Change-Number: 10918
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jul 2018 00:04:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/10918 )
Change subject: IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
......................................................................
IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
In order to do COMPUTE STATS, Impala performs several SELECT queries.
However, in the COMPUTE STATS analysis phase, we only register for the
ALTER privilege. Although the SELECT privilege is eventually registered
to the target table by the SELECT child queries, it is better to
register the SELECT privilege in the COMPUTE STATS analysis phase and
fail early if the privilege requirements are not met instead of failing
later in the SELECT child queries due to insufficient privileges.
Testing:
- Updated the authorization tests
- Ran all FE tests
Change-Id: Icead43e689404a286859344e309427eb6c68646a
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionDef.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
7 files changed, 91 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/10918/3
--
To view, visit http://gerrit.cloudera.org:8080/10918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icead43e689404a286859344e309427eb6c68646a
Gerrit-Change-Number: 10918
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10918 )
Change subject: IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/10918/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10918/3//COMMIT_MSG@10
PS3, Line 10: check fo
> the word "register" stuck out. seems like an implementation detail. would i
Makes sense. Done.
http://gerrit.cloudera.org:8080/#/c/10918/3/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/10918/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2389
PS3, Line 2389:
> add a comment clarifying whether all or some privilege is required when mul
Done
http://gerrit.cloudera.org:8080/#/c/10918/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2393
PS3, Line 2393: Preconditions.checkNotNull(privilege);
> nit: space before ":" (for consistency with this file)
Done
http://gerrit.cloudera.org:8080/#/c/10918/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2408
PS3, Line 2408: TCatalogObjectType objectType = TCatalogObjectType.TABLE;
> nit: space before ":"
Done
http://gerrit.cloudera.org:8080/#/c/10918/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:
http://gerrit.cloudera.org:8080/#/c/10918/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@1166
PS3, Line 1166: TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT
> to avoid unrolling the loop over compute/drop, would it be straightforward
There are test cases that are not applicable to "drop stats" if we don't unroll the loop. For example:
.error(alterError("functional.alltypes"), onServer(allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT))
--
To view, visit http://gerrit.cloudera.org:8080/10918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icead43e689404a286859344e309427eb6c68646a
Gerrit-Change-Number: 10918
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Jul 2018 23:38:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10918 )
Change subject: IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
......................................................................
Patch Set 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/10918/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10918/3//COMMIT_MSG@10
PS3, Line 10: register
the word "register" stuck out. seems like an implementation detail. would it be equivalent to simply replace it with "check"?
http://gerrit.cloudera.org:8080/#/c/10918/3/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/10918/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2389
PS3, Line 2389: Privilege... privilege
add a comment clarifying whether all or some privilege is required when multiple privs are specified.
http://gerrit.cloudera.org:8080/#/c/10918/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2393
PS3, Line 2393: for (Privilege priv: privilege) {
nit: space before ":" (for consistency with this file)
http://gerrit.cloudera.org:8080/#/c/10918/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2408
PS3, Line 2408: for (Privilege priv: privilege) {
nit: space before ":"
http://gerrit.cloudera.org:8080/#/c/10918/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:
http://gerrit.cloudera.org:8080/#/c/10918/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@1166
PS3, Line 1166: TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT
to avoid unrolling the loop over compute/drop, would it be straightforward to loop over [(op, exp_priv)]?
for this case, that would be:
[("compute", [TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT]),
("drop", [TPrivilegeLevel.ALTER])]
--
To view, visit http://gerrit.cloudera.org:8080/10918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icead43e689404a286859344e309427eb6c68646a
Gerrit-Change-Number: 10918
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Jul 2018 23:21:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10918 )
Change subject: IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icead43e689404a286859344e309427eb6c68646a
Gerrit-Change-Number: 10918
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jul 2018 00:04:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/10918 )
Change subject: IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
......................................................................
IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
In order to do COMPUTE STATS, Impala performs several SELECT queries.
However, in the COMPUTE STATS analysis phase, we only register for the
ALTER privilege. Although the SELECT privilege is eventually registered
to the target table by the SELECT child queries, it is better to register
the SELECT privilege in the COMPUTE STATS analysis phase and fail early
if the privilege requirements are not met instead of failing later in the
SELECT child queries due to insufficient privileges.
Testing:
- Updated the authorization tests
- Ran all FE tests
Change-Id: Icead43e689404a286859344e309427eb6c68646a
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionDef.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
7 files changed, 91 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/10918/2
--
To view, visit http://gerrit.cloudera.org:8080/10918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icead43e689404a286859344e309427eb6c68646a
Gerrit-Change-Number: 10918
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10918 )
Change subject: IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10918/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10918/1//COMMIT_MSG@15
PS1, Line 15: SELCT
nit: SELECT
http://gerrit.cloudera.org:8080/#/c/10918/1/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/10918/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2384
PS1, Line 2384: Always registers a privilege request for the table at the given privilege level,
: * regardless of the state of the table (i.e. whether it exists, is loaded, etc.).
: * If addAccessEvent is true adds an access event for successfully loaded tables.
nit: Update comment to reflect multiple privileges and access events.
http://gerrit.cloudera.org:8080/#/c/10918/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:
http://gerrit.cloudera.org:8080/#/c/10918/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@1185
PS1, Line 1185: default.nodb"), onDatabase("nodb", allExcept(
: TPrivilegeLevel.ALL, TPrivilegeLevel.SELECT)
missing TPrivilegeLevel.ALTER - alterError("nodb.notbl")
http://gerrit.cloudera.org:8080/#/c/10918/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@1193
PS1, Line 1193: default.functional"), onDatabase("functional", allExcept(
: TPrivilegeLevel.ALL, TPrivilegeLevel.SELECT)
same as above - alterError("functional.notbl")
http://gerrit.cloudera.org:8080/#/c/10918/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@1217
PS1, Line 1217: default.nodb"), onDatabase("nodb", allExcept(
: TPrivilegeLevel.ALL)));
same as above - alterError("nodb.notbl")
http://gerrit.cloudera.org:8080/#/c/10918/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@1225
PS1, Line 1225: ("default.functional"), onDatabase("functional", allExcept(
: TPrivilegeLevel.ALL)));
same as above - alterError("functional.notbl")
--
To view, visit http://gerrit.cloudera.org:8080/10918
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icead43e689404a286859344e309427eb6c68646a
Gerrit-Change-Number: 10918
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Jul 2018 20:32:25 +0000
Gerrit-HasComments: Yes