impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1670,IMPALA-4141: Support multiple partitions in ALTER TABLE ADD PARTITION
Date Thu, 27 Oct 2016 18:33:43 GMT
Dimitris Tsirogiannis has posted comments on this change.

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


Patch Set 19: Code-Review+1

(8 comments)

Minor fixes in the tests. I'll ask Alex to see if he can make a final pass.

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

PS19, Line 73: List<TPartitionParams> partParams = Lists.newArrayList();
             :     for (PartitionParams p: partitions_) partParams.add(p.toThrift());
I think you can simply use TAlterTableParams.AddToPartition_params(). It already handles the
initialization of the list and can save you a few precious lines :)


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

PS19, Line 211:  
nit: this is just for comparison purposes, no need to pretty print (save a few bytes :)).


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

PS19, Line 1810: addedHmsPartitions.addAll(getPartitionsFromHms(msClient, difference));
Let's try to be a bit more defensive here. If getPartiionsFromHms() throws an exception we
should inform the user that the some metadata inconsistency has occurred and the user should
run INVALIDATE METADATA <tbl> to recover.


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

PS19, Line 963: drop table if exists i1670_alter;
You don't need the drop table statements. Plz remove


PS19, Line 987: Duplicate partition spec: (j=11, n='alex', i=1)
Isn't this an analysis exception? I believe it will be an ImpalaRuntimeException which you
can catch here if "IF NOT EXISTS" is not used.


PS19, Line 990: # IMPALA-1670: IF NOT EXISTS works as expected:
              : # - ignores existing partitions and adds the rest.
              : # - location and caching options of existing partitions are not modified.
              : # - if adding one partition fails, the entire statement fails.
The query that follows this comment does not use the IF NOT EXISTS clause which is confusing.
You may want to remove this and add a comment before each ALTER TABLE stmt that indicates
what's being currently tested.


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

PS19, Line 242: # IMPALA-1670: Revoke privileges, so that user would not have privileges to
run ALTER TABLE
nit: long line


http://gerrit.cloudera.org:8080/#/c/4144/19/tests/metadata/test_hms_integration.py
File tests/metadata/test_hms_integration.py:

PS19, Line 733: rows
nit: with_data


-- 
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: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <attilaj@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