Attila Jeges has posted comments on this change. Change subject: IMPALA-1670: Support multiple partitions in ALTER TABLE ADD PARTITION ......................................................................
Patch Set 5: (21 comments) Thanks for commenting o this change. IMPALA-1654 addresses a different improvement, merging the two changes together shuld not be an issue. 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 Done PS4, Line 231: add_partitions > nit: maybe call it simply partitions or partitions_to_add? Done 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 f Done PS4, Line 879: ArrayList<AddPartition> list = new ArrayList<AddPartition>(); : list.add(item); > I believe you can replace these two lines with one: Done 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? Removed hdfsTable_ PS4, Line 105: a > "an" Done 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? I guess it was exposed in an earlier draft, but it is not needed anymore. Removing ImmutableList. PS4, Line 43: ifNotExists > As I mentioned in an earlier comment, plz describe the semantics of using t Done PS4, Line 46: partitions.size() > 0 > !partitions.isEmpty() Done PS4, Line 47: mmutableList.copyOf(partitions); > I don't believe you need a copy. Done PS4, Line 49: > formatting nit: no space between variable name and ':'. Also, don't this bl Done PS4, Line 60: if (ifNotExists_) { : sb.append(" IF NOT EXISTS"); : } > single line, no braces Done PS4, Line 63: for (AddPartition p : partitions_) { : sb.append(" " + p.toSql()); : } > here as well (single line, no braces) and everywhere this applies :) Done PS4, Line 97: .toLowerCase > Are you sure the partition key values are not already in lower case? I've looked it up, they are converted to lower case in PartitionKeyValue class. Removing toLowerCase() call from here. 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? refreshedTable variable is also used in L400 in the DROP_PARTITION case block. Since refreshedTable is used in different case blocks, I thought it would be cleaner to define it in front of the switch statement. I can move it back to L375 of course, if you think that it does not hurt readability. 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 cas Done PS4, Line 1641: Table result = null; > I don't think you need this. You can remove it and have L1707 simply do "re Done 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: Done 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 Fixed PS4, Line 1015: test-warehouse/i1670_alter > Why using the entire path here? Done 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 compare Ok, removing it. -- 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: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Attila Jeges <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
