Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-1670: Support multiple partitions in ALTER TABLE ADD PARTITION ......................................................................
Patch Set 4: (21 comments) Nice work. You may want to add some simple end to end authorization tests as well as an integration test (e.g. create table in impala, add partitions using hive, try to add multiple partitions using Impala, some may already exist, make sure the catalog does the right thing). http://gerrit.cloudera.org:8080/#/c/4144/4/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: PS4, Line 227: If true, no error is raised if a partition with the same spec already exists. What happens when multiple partitions are specified, if_not_exists is true and some of the inserted partitions already exist? Does the statement ignore those that exist and insert the rest or does the statement simply return without an error? Can you clarify that behavior somewhere and maybe update the comment here as well? PS4, Line 231: add_partitions nit: maybe call it simply partitions or partitions_to_add? http://gerrit.cloudera.org:8080/#/c/4144/4/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: PS4, Line 377: add_partition; : nonterminal ArrayList<AddPartition> add_partition_list; 'AddPartition', 'add_partition' and 'add_partition_list' sound a lot like function names. Can we change them to something more noun-like :)? e.g. PartitionParams instead of AddPartitions, etc... PS4, Line 879: ArrayList<AddPartition> list = new ArrayList<AddPartition>(); : list.add(item); I believe you can replace these two lines with one: ArrayList<AddPartition> list = Lists.newArrayList(item); http://gerrit.cloudera.org:8080/#/c/4144/4/fe/src/main/java/com/cloudera/impala/analysis/AddPartition.java File fe/src/main/java/com/cloudera/impala/analysis/AddPartition.java: PS4, Line 41: private Table table_; : private HdfsTable hdfsTable_; Why do you need both? PS4, Line 105: a "an" http://gerrit.cloudera.org:8080/#/c/4144/4/fe/src/main/java/com/cloudera/impala/analysis/AlterTableAddPartitionStmt.java File fe/src/main/java/com/cloudera/impala/analysis/AlterTableAddPartitionStmt.java: PS4, Line 40: ImmutableList Why an ImmutableList? Do you expose this outside the class? PS4, Line 43: ifNotExists As I mentioned in an earlier comment, plz describe the semantics of using this flag in the context of adding multiple partitions. PS4, Line 46: partitions.size() > 0 !partitions.isEmpty() PS4, Line 47: mmutableList.copyOf(partitions); I don't believe you need a copy. PS4, Line 49: formatting nit: no space between variable name and ':'. Also, don't this block fit in a single line? PS4, Line 60: if (ifNotExists_) { : sb.append(" IF NOT EXISTS"); : } single line, no braces PS4, Line 63: for (AddPartition p : partitions_) { : sb.append(" " + p.toSql()); : } here as well (single line, no braces) and everywhere this applies :) PS4, Line 97: .toLowerCase Are you sure the partition key values are not already in lower case? http://gerrit.cloudera.org:8080/#/c/4144/4/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: PS4, Line 358: Table refreshedTable = null; Why is this here? Can you move it back to L375? PS4, Line 373: If the partitions already exist in Hive and "IfNotExists" is true As I commented earlier, let's try to clarify a bit the behavior in this case when there are partial conflicts. What are the possible outcomes? all/partial/none is added? PS4, Line 1641: Table result = null; I don't think you need this. You can remove it and have L1707 simply do "return addHdfsPartition(tbl, partition);" http://gerrit.cloudera.org:8080/#/c/4144/4/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java: Line 204: Can you also add the following cases: 1. multiple partitions with locations and caching (ok) 2. one of the partitions points to an invalid URI (error) http://gerrit.cloudera.org:8080/#/c/4144/4/testdata/workloads/functional-query/queries/QueryTest/alter-table.test File testdata/workloads/functional-query/queries/QueryTest/alter-table.test: PS4, Line 1004: pzrtitions typo PS4, Line 1015: test-warehouse/i1670_alter Why using the entire path here? PS4, Line 1071: ---- QUERY : alter table i1670_alter add if not exists : partition (i=0) location '/i0C' : partition (i=1) location '/i1C' : partition (i=2) location '/i2C'; : show partitions i1670_alter; : ---- RESULTS : '0',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i0C : '1',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i1A : '2',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i2B : '3',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i3A : '4',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i4B : 'Total',-1,0,'0B','0B','','','','' : ---- TYPES : STRING, BIGINT, BIGINT, STRING, STRING, STRING, STRING, STRING, STRING : ==== : ---- QUERY : alter table i1670_alter add if not exists : partition (i=1) location '/i1D' : partition (i=2) location '/i2D' : partition (i=3) location '/i3D'; : show partitions i1670_alter; : ---- RESULTS : '0',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i0C : '1',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i1A : '2',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i2B : '3',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i3A : '4',-1,0,'0B','NOT CACHED','NOT CACHED','TEXT','false',regex:.*/i4B : 'Total',-1,0,'0B','0B','','','','' : ---- TYPES : STRING, BIGINT, BIGINT, STRING, STRING, STRING, STRING, STRING, STRING I don't think these cases add anything in terms of testing coverage compared to the case in L1056-1067 -- 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Attila Jeges <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
