impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Attila Jeges (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1670,IMPALA-4141: Support multiple partitions in ALTER TABLE ADD PARTITION
Date Wed, 28 Sep 2016 18:56:42 GMT
Attila Jeges has posted comments on this change.

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


Patch Set 14:

(15 comments)

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

PS13, Line 1630: IF NOT EXISTS is not used and there is a c
> "IF NOT EXISTS is used "
Done


PS13, Line 1683: 
> "fail"
Done


PS13, Line 1684: tions,
> "In that case, we"
Done


PS13, Line 1687: try {
               :         updateLastDdlTime(msTbl, msClient);
               :       } catch (TException e) {
               :         throw new ImpalaRuntimeException(
               :             String.format(HMS_RPC_ERROR_FORMAT_STR, "updateLastDdlTime"),
e);
               :       }
> The semantics of getPartitionFromHMS() (L1720) and the way it is used here 
Done


PS13, Line 1715: table. Partition objects are distinguished by partition values only.
               :    */
               :   private List<Partition> computeDifference(List<Partition> aList,
               :       List<Partition> bList) {
               :     Set<List<String>> bSet = Sets.newHashSet();
               :     for (Partition b: bList) bSet.add(b.getValues());
               : 
> This function and the way it is used is kind of confusing. What you essenti
Done


http://gerrit.cloudera.org:8080/#/c/4144/13/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java:

PS13, Line 205: 
              :   @Test
              :   public void TestAlterTableAddMultiplePartitions() {
              :     for (String cl: new String[]{"if not exists", ""}) {
              :       // Add multiple partitions.
              :       AnalyzesOk("alter table functional.alltypes add " + cl +
              :           " partition(year=2050, month=10)" +
              :           " partition(year=2050, month=11)" +
              :           " partition(year=2050, month=12)");
              :       // Duplicate partition specifications.
              :       AnalysisError("alter table functional.alltypes add " + cl +
              :           " partition(year=2050, month=10)" +
              :           " partition(year=2050, month=11)" +
              :           " partition(Month=10, YEAR=2050)",
              :           "Duplicate partition spec: (month=10, year=2050)");
              :       AnalysisError("alter table functional.alltypes add " + cl +
              :           " partition(year=2050, month=10)" +
              :           " partition(year=2050, month=11)" +
              :           " partition(Month=10, YEAR=2050) location" +
              :           " 'hdfs://localhost:20500/test-warehouse/alltypes/year=2010/month=10'",
              :           "Duplicate partition spec: (month=10, year=2050)");
              : 
              :       // Multiple partitions with locations and caching
              :       AnalyzesOk("alter table functional.alltypes add " + cl +
              :           " partition(year=2050, month=10) location" +
              :           " '/test-warehouse/alltypes/y2050m10' cached in 'testPool'" +
              :           " partition(year=2050, month=11) location" +
              :           " 'hdfs://localhost:20500/test-warehouse/alltypes/y2050m11'" +
              :           " cached in 'testPool' with replication = 7" +
              :           " partition(year=2050, month=12) location" +
              :           " 'file:///test-warehouse/alltypes/y2050m12' uncached");
              :       // One of the partitions points to an invalid URI
              :       AnalysisError("alter table functional.alltypes add " + cl +
              :           " partition(year=2050, month=10) location" +
              :           " '/test-warehouse/alltypes/y2050m10' cached in 'testPool'" +
              :           " partition(year=2050, month=11) location" +
              :           " 'hdfs://localhost:20500/test-warehouse/alltypes/y2050m11'" +
              :           " cached in 'testPool' with replication = 7" +
              :           " partition(year=2050, month=12) location" +
              :           " 'fil:///test-warehouse/alltypes/y2050m12' uncached",
              :           "No FileSystem for scheme: fil");
              :     }
> One easy way to increase test coverage could be to parameterize these test 
Done. Moved new test cases to TestAlterTableAddMultiplePartitions() function


http://gerrit.cloudera.org:8080/#/c/4144/13/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
File fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java:

Line 1236: 
> Also, can you add a test case where the user doesn't have privileges to add
There is already a test like that on L1255


http://gerrit.cloudera.org:8080/#/c/4144/13/fe/src/test/java/com/cloudera/impala/analysis/ToSqlTest.java
File fe/src/test/java/com/cloudera/impala/analysis/ToSqlTest.java:

PS13, Line 1038: locatio
> Also a partition with location
Done


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

PS13, Line 1037: ====
> Another test case to consider is adding partitions to a table that is alrea
Done


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

PS13, Line 256: grant role grant_revoke_test_ALL_TEST_TBL to G
> Add also a case where user doesn't have permissions to add partitions to a 
Done


http://gerrit.cloudera.org:8080/#/c/4144/13/tests/common/impala_test_suite.py
File tests/common/impala_test_suite.py:

PS13, Line 188:   """
> There is a lot of duplicate code between this function and impala_partition
Done. The new method is called impala_partitions()


PS13, Line 208:       for i in fields_idx:
> Plz add comments. It's not clear to me how to use this function.
Added comments and moved it back to test_hms_integration.py


http://gerrit.cloudera.org:8080/#/c/4144/13/tests/metadata/test_partition_metadata.py
File tests/metadata/test_partition_metadata.py:

PS13, Line 118: 
> I think some of the test cases in this function probably belong to the hms_
All of the test cases in this method are about HMS integration. Moved it to test_hms_integration.py.


Line 121
> You also need to add some additional cases:
Added the required tests to test_hms_integration.py (test_add_preexisting_partitions_with_rows()
and test_partitions_exist_after_refresh())


PS13, Line 148: 
> I don't think this is correct. You essentially updated partition x=2. Think
Fixed the issue and added new test to test_hms_integration.py (test_add_overlapping_partitions())


-- 
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: 14
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