impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-1654: General partition exprs in DDL operations.
Date Thu, 15 Sep 2016 00:32:40 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-1654: General partition exprs in DDL operations.

Patch Set 11:


Amos, sorry to meddle with your patch but it seemed easier/faster this way since I have no
idea what's wrong with your env.

I fixed the AuthorizationTest and AuditingTest for you. Please look at the changes carefully
and try to apply the same principles in other places in your patch.

I added comments to explain what the bug was.

I'm still looking at those other failing tests. Hopefully we can fix your env so you can repro
File fe/src/main/java/com/cloudera/impala/analysis/

Line 70:   public void analyze(Analyzer analyzer) throws AnalysisException {
The changes here lead to a bunch of changes in error messages which I think are fine.

Can you fix the expected error messages in the FE tests, e.g., AnalyzeDDLTest?

Line 72:     TableRef tableRef = new TableRef(tableName_.toPath(), null, Privilege.ALTER);
This version is better because:
* we only ask the catalog for this table once, avoiding potential inconsistency problems
* we register the appropriate audit and auth events exactly once in tableRef.analyze()
* more streamlined code

Line 80:     if (tableRef instanceof CollectionTableRef) {
Could you add a test for this?
File fe/src/main/java/com/cloudera/impala/analysis/

Line 61:     TableRef tableRef = new TableRef(tableName_.toPath(), null, Privilege.VIEW_METADATA);
I'm explaining the auth/audit bug in the following comments.

Line 65:     if (!(table_ instanceof HdfsTable)) {
This registers audit and auth events for the VIEW_METADATA privilege.

Line 74:       partitionSet_.setPartitionShouldExist();
This registers audit and auth events for the SELECT privilege - which is wrong and caused
tests to fail.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I2c9162fcf9d227b8daf4c2e761d57bab4e26408f
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Amos Bird <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Amos Bird <>
Gerrit-HasComments: Yes

View raw message