----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27917/#review61410 -----------------------------------------------------------
Mostly looks good. Just some minor comment/question below. Thanks again for this patch! metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java <https://reviews.apache.org/r/27917/#comment102977> Please fill this out. metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java <https://reviews.apache.org/r/27917/#comment102979> Can we use the FieldSchema.equals method here? ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java <https://reviews.apache.org/r/27917/#comment102984> Let's just "not" this statement for conciseness. ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java <https://reviews.apache.org/r/27917/#comment102982> Please fix the indent here of inside the switch statement. ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java <https://reviews.apache.org/r/27917/#comment102983> Do we need 'restrict'? What does it do? - Szehon Ho On Nov. 14, 2014, 3:02 a.m., Chaoyu Tang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27917/ > ----------------------------------------------------------- > > (Updated Nov. 14, 2014, 3:02 a.m.) > > > Review request for hive. > > > Repository: hive-git > > > Description > ------- > > We often run into some issues like HIVE-6131which is due to inconsistent > column descriptors between table and partitions after alter table. > HIVE-8441/HIVE-7971 provided the flexibility to alter table at partition > level. But most cases we have need change the table and partitions at same > time. In addition, "alter table" is usually required prior to "alter table > partition .." since querying table partition data is also through table. > Instead of do that in two steps, here we provide a convenient ddl like "alter > table ... cascade" to cascade table changes to partitions as well. The > changes are only limited and applicable to add/replace columns and change > column name, datatype, position and comment. > > Highlights of changes: > 1. Add support to sql syntax: > ALTER TABLE table_name [PARTITION partition_spec] CHANGE [COLUMN] > col_old_name col_new_name column_type > [COMMENT col_comment] [FIRST|AFTER column_name] [CASCADE|RESTRICT] > ALTER TABLE table_name [PARTITION partition_spec] ADD|REPLACE COLUMNS > (col_name data_type [COMMENT col_comment], ...) [CASCADE|RESTRICT] > > default is RESTRICT > 2. Add metastore Thrift API to support the alter table with cascade option > void alter_table_with_cascade(1:string dbname, 2:string tbl_name, 3:Table > new_tbl, 4:bool cascade) throws (1:InvalidOperationException o1, > 2:MetaException o2) > > So there will be some generated thrift code (C++, php, py, rb) included in > this patch > > 3. new test alter_table_cascade.q > > > Diffs > ----- > > metastore/if/hive_metastore.thrift 462580179c3484a645aaccc0ad37105b36f17a5b > metastore/src/java/org/apache/hadoop/hive/metastore/AlterHandler.java > d872be5d8af60ff933729bf390ed83887a04518c > metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java > fc6215a8b1baeda046ff661ce7279cdd9bf2413f > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > a47619c9c2660505f973ef809b36041b66c31bdd > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java > 6a0cb96640a303638d159ed141d3996ccb9bfe98 > metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java > 066ab68443fdd1bd04e14c8a3b1b2bbac9130d6c > metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java > 1ac5affc51ca5cab59eacdc333c6b344ea384887 > ql/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java > 292c83cf967756ad3056f8a57de9590a0b00babf > ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java > 1655f3de2df32a890030b0f148155115d7d48b2e > ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java > b90062704edb0024b2a08b83e4d351d4ef4a14a3 > > ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java > f1f723c939b42fb441ddeaa4159249d897203a08 > ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java > b105424b25ede962af3060daf915b47866fd941c > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g > f1365fa3938cd6505bcacf005a077ebb1950c427 > ql/src/java/org/apache/hadoop/hive/ql/plan/AlterTableDesc.java > f869821d2c928893190d8063c37708183e5eba20 > ql/src/test/queries/clientpositive/alter_table_cascade.q PRE-CREATION > ql/src/test/results/clientpositive/alter_table_cascade.q.out PRE-CREATION > > Diff: https://reviews.apache.org/r/27917/diff/ > > > Testing > ------- > > 1. qtests alter_table_cascade.q passed > 2. some manual tests with both embedded and remote HMS > 3. will submit for pre-commit tests > > > Thanks, > > Chaoyu Tang > >
