impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1670,IMPALA-4141: Support multiple partitions in ALTER TABLE ADD PARTITION
Date Thu, 26 Jan 2017 01:20:31 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-1670,IMPALA-4141: Support multiple partitions in ALTER TABLE ADD PARTITION
......................................................................


Patch Set 22:

(10 comments)

Thanks for the clarifications. Change looks pretty good to me.

http://gerrit.cloudera.org:8080/#/c/4144/22/fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java:

Line 92:       throw new AnalysisException(
Add a test for this in AnalyzeDDLTest


http://gerrit.cloudera.org:8080/#/c/4144/22/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

Line 1859:    * 2. If a partition exists in HMS but not in catalog cache, reload partition
from HMS.
mention that caching directives are only applied to new partitions that were absent from both
the catalog cache and the HMS


Line 1861:   private Table alterTableAddPartitions(Table tbl,
Since there are some interesting details with existing partitions (in cache or HMS), I think
it would be nice to return the number of partitions actually created to the user. I'm ok with
doing that in a follow-on patch. If you decide to go that route, please file a JIRA.


http://gerrit.cloudera.org:8080/#/c/4144/22/testdata/workloads/functional-query/queries/QueryTest/alter-table.test
File testdata/workloads/functional-query/queries/QueryTest/alter-table.test:

Line 1023: # IMPALA-1670: Cannot add duplicate partition specs.
remove, this is covered by an analyzer test


Line 1033: # IMPALA-1670: Cannot add duplicate partition specs, not even if 'IF NOT EXISTS'
is used.
remove, this is covered by an analyzer test


Line 1056: # IMPALA-1670: If 'IF NOT EXISTS' is not used, ALTER TABLE ADD PARTITION cannot
add a
This should be covered by an analyzer test instead


Line 1066: # IMPALA-1670: If adding one partition fails, the entire statement fails.
Not really accurate. There are also modes of partial failure/success. If analysis fails, it's
clear no state should have changed (because the stmt does not go into runtime).


Line 1076: # IMPALA-1670: If 'IF NOT EXISTS' is used ALTER TABLE ADD PARTITION works with
preexisting
Nice!


Line 1150: # IMPALA-1670: After REFRESH Impala can access previously added partitions and
partition
Seems a little excessive to me, not sure if we need this test.


http://gerrit.cloudera.org:8080/#/c/4144/22/testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test
File testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test:

Line 242: # IMPALA-1670: Revoke privileges, so that user would not have privileges to run
ALTER
These cases should be covered by AuthorizationTest already. I don't think we need to test
grant/revoking and then performing this ALTER TABLE specifically. What do you think, Dimitris?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iddbc951f2931f488f7048c9780260f6b49100750
Gerrit-PatchSet: 22
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message