You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Miklos Gergely <mg...@hortonworks.com> on 2019/05/15 07:31:58 UTC
Review Request 70646: Break up DDLTask - extract Column and Constraint
related operations
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70646/
-----------------------------------------------------------
Review request for hive and Zoltan Haindrich.
Bugs: HIVE-21725
https://issues.apache.org/jira/browse/HIVE-21725
Repository: hive-git
Description
-------
DDLTask is a huge class, more than 5000 lines long. The related DDLWork is also a huge class, which has a field for each DDL operation it supports. The goal is to refactor these in order to have everything cut into more handleable classes under the package org.apache.hadoop.hive.ql.exec.ddl:
have a separate class for each operation
have a package for each operation group (database ddl, table ddl, etc), so the amount of classes under a package is more manageable
make all the requests (DDLDesc subclasses) immutable
DDLTask should be agnostic to the actual operations
right now let's ignore the issue of having some operations handled by DDLTask which are not actual DDL operations (lock, unlock, desc...)
In the interim time when there are two DDLTask and DDLWork classes in the code base the new ones in the new package are called DDLTask2 and DDLWork2 thus avoiding the usage of fully qualified class names where both the old and the new classes are in use.
Step #9: extract all the column and constraint related operations from the old DDLTask, and move them under the new package each.
Diffs
-----
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/AlterTableUtils.java 3c6d7eada9
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/AlterTableWithConstraintsDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/AlterTableWithWriteIdDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/AlterTableWithWriteIdOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableAddColumnsDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableAddColumnsOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableChangeColumnDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableChangeColumnOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableReplaceColumnsDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableReplaceColumnsOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableUpdateColumnsDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableUpdateColumnsOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/ShowColumnsOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/package-info.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/constaint/AlterTableAddConstraintDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/constaint/AlterTableAddConstraintOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/constaint/AlterTableDropConstraintDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/constaint/AlterTableDropConstraintOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/constaint/Constraints.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/constaint/package-info.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/info/DescTableDesc.java cdd1777767
ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 0c531bed51
ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 99d7f21228
ql/src/java/org/apache/hadoop/hive/ql/parse/repl/load/message/AddForeignKeyHandler.java bba769244b
ql/src/java/org/apache/hadoop/hive/ql/parse/repl/load/message/AddNotNullConstraintHandler.java 90d9008a31
ql/src/java/org/apache/hadoop/hive/ql/parse/repl/load/message/AddPrimaryKeyHandler.java e8966ad7c4
ql/src/java/org/apache/hadoop/hive/ql/parse/repl/load/message/AddUniqueConstraintHandler.java 81f1c5ab20
ql/src/java/org/apache/hadoop/hive/ql/parse/repl/load/message/DropConstraintHandler.java 5f9f879f6f
ql/src/java/org/apache/hadoop/hive/ql/plan/AlterTableDesc.java 8603521041
ql/src/java/org/apache/hadoop/hive/ql/plan/DDLWork.java 6cd84bb8ab
ql/src/java/org/apache/hadoop/hive/ql/plan/ShowColumnsDesc.java 7047f56275
ql/src/test/queries/clientpositive/allow_change_col_type_par.q aad63705f7
ql/src/test/queries/clientpositive/avro_alter_table_update_columns.q 279d05d2e3
ql/src/test/queries/clientpositive/check_constraint.q 202110259d
ql/src/test/queries/clientpositive/rename_column.q 96daf9d658
ql/src/test/queries/clientpositive/show_columns.q aa45bae9d5
ql/src/test/results/clientnegative/allow_change_col_type_par_neg.q.out 3f91e85a3a
ql/src/test/results/clientnegative/alter_partition_change_col_dup_col.q.out 542e85cb89
ql/src/test/results/clientnegative/alter_partition_change_col_nonexist.q.out bc5a6f980f
ql/src/test/results/clientnegative/alter_table_constraint_duplicate_pk.q.out acf65f2ff6
ql/src/test/results/clientnegative/alter_table_constraint_invalid_fk_col1.q.out 1617609ce2
ql/src/test/results/clientnegative/alter_table_constraint_invalid_fk_col2.q.out 47166ac6c2
ql/src/test/results/clientnegative/alter_table_constraint_invalid_fk_tbl1.q.out 49bc928ac1
ql/src/test/results/clientnegative/alter_table_constraint_invalid_fk_tbl2.q.out f5ac4ac54a
ql/src/test/results/clientnegative/alter_table_constraint_invalid_pk_col.q.out 71689f70ee
ql/src/test/results/clientnegative/alter_table_constraint_invalid_pk_tbl.q.out 792134cbe6
ql/src/test/results/clientnegative/alter_table_constraint_invalid_ref.q.out 9e98454f5b
ql/src/test/results/clientnegative/altern1.q.out ff3c670864
ql/src/test/results/clientnegative/avro_add_column_extschema.q.out a1b1b9ca6e
ql/src/test/results/clientnegative/column_rename1.q.out 01549c2665
ql/src/test/results/clientnegative/column_rename2.q.out 41c2219d10
ql/src/test/results/clientnegative/column_rename4.q.out d5729f127a
ql/src/test/results/clientnegative/disallow_incompatible_type_change_on1.q.out 8fe4c05872
ql/src/test/results/clientnegative/disallow_incompatible_type_change_on2.q.out 6291feb931
ql/src/test/results/clientnegative/drop_invalid_constraint1.q.out 2cb3996015
ql/src/test/results/clientnegative/drop_invalid_constraint2.q.out 04352b40d7
ql/src/test/results/clientnegative/drop_invalid_constraint3.q.out 03e4bd6097
ql/src/test/results/clientnegative/drop_invalid_constraint4.q.out 473dec7a4c
ql/src/test/results/clientnegative/hms_using_serde_alter_table_update_columns.q.out 202acd7e31
ql/src/test/results/clientnegative/orc_reorder_columns1.q.out c581f4e312
ql/src/test/results/clientnegative/orc_reorder_columns1_acid.q.out 8f7255c973
ql/src/test/results/clientnegative/orc_reorder_columns2.q.out 54dcdecf1c
ql/src/test/results/clientnegative/orc_reorder_columns2_acid.q.out 6cae15b81a
ql/src/test/results/clientnegative/orc_replace_columns1.q.out 13f3f1448b
ql/src/test/results/clientnegative/orc_replace_columns1_acid.q.out 46caec214f
ql/src/test/results/clientnegative/orc_replace_columns2.q.out 2316bbbf35
ql/src/test/results/clientnegative/orc_replace_columns2_acid.q.out e01b7b9edb
ql/src/test/results/clientnegative/orc_replace_columns3.q.out a7b3b72ced
ql/src/test/results/clientnegative/orc_replace_columns3_acid.q.out b82ad57f6e
ql/src/test/results/clientnegative/orc_type_promotion1.q.out f45283664a
ql/src/test/results/clientnegative/orc_type_promotion1_acid.q.out 49800a96df
ql/src/test/results/clientnegative/orc_type_promotion2.q.out 740ee1e850
ql/src/test/results/clientnegative/orc_type_promotion2_acid.q.out 28c789a20a
ql/src/test/results/clientnegative/orc_type_promotion3.q.out 4f97e3118e
ql/src/test/results/clientnegative/orc_type_promotion3_acid.q.out a214985f94
ql/src/test/results/clientnegative/parquet_alter_part_table_drop_columns.q.out d22d9c8763
ql/src/test/results/clientpositive/allow_change_col_type_par.q.out e9570370ea
ql/src/test/results/clientpositive/avro_alter_table_update_columns.q.out 7f74f6c141
ql/src/test/results/clientpositive/input3.q.out 0ac95780cb
ql/src/test/results/clientpositive/llap/check_constraint.q.out 297d8928f8
ql/src/test/results/clientpositive/rename_column.q.out 43abc7f72a
ql/src/test/results/clientpositive/show_columns.q.out 80d69a7276
Diff: https://reviews.apache.org/r/70646/diff/1/
Testing
-------
Added new unit tests + all the old unit tests are still running fine.
Thanks,
Miklos Gergely
Re: Review Request 70646: Break up DDLTask - extract Column and
Constraint related operations
Posted by Miklos Gergely <mg...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70646/
-----------------------------------------------------------
(Updated May 27, 2019, 12:07 p.m.)
Review request for hive and Zoltan Haindrich.
Bugs: HIVE-21725
https://issues.apache.org/jira/browse/HIVE-21725
Repository: hive-git
Description
-------
DDLTask is a huge class, more than 5000 lines long. The related DDLWork is also a huge class, which has a field for each DDL operation it supports. The goal is to refactor these in order to have everything cut into more handleable classes under the package org.apache.hadoop.hive.ql.exec.ddl:
have a separate class for each operation
have a package for each operation group (database ddl, table ddl, etc), so the amount of classes under a package is more manageable
make all the requests (DDLDesc subclasses) immutable
DDLTask should be agnostic to the actual operations
right now let's ignore the issue of having some operations handled by DDLTask which are not actual DDL operations (lock, unlock, desc...)
In the interim time when there are two DDLTask and DDLWork classes in the code base the new ones in the new package are called DDLTask2 and DDLWork2 thus avoiding the usage of fully qualified class names where both the old and the new classes are in use.
Step #9: extract all the column and constraint related operations from the old DDLTask, and move them under the new package each.
Diffs (updated)
-----
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/AbstractAlterTableDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/AbstractAlterTableOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/AbstractAlterTableWithConstraintsDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/AlterTableUtils.java 3c6d7eada9
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableAddColumnsDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableAddColumnsOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableChangeColumnDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableChangeColumnOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableReplaceColumnsDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableReplaceColumnsOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableUpdateColumnsDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/AlterTableUpdateColumnsOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/ShowColumnsOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/package-info.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/constaint/AlterTableAddConstraintDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/constaint/AlterTableAddConstraintOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/constaint/AlterTableDropConstraintDesc.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/constaint/AlterTableDropConstraintOperation.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/constaint/Constraints.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/constaint/package-info.java PRE-CREATION
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/info/DescTableDesc.java cdd1777767
ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 0c531bed51
ql/src/java/org/apache/hadoop/hive/ql/hooks/WriteEntity.java 3afa201fbc
ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 99d7f21228
ql/src/java/org/apache/hadoop/hive/ql/parse/repl/load/message/AddForeignKeyHandler.java bba769244b
ql/src/java/org/apache/hadoop/hive/ql/parse/repl/load/message/AddNotNullConstraintHandler.java 90d9008a31
ql/src/java/org/apache/hadoop/hive/ql/parse/repl/load/message/AddPrimaryKeyHandler.java e8966ad7c4
ql/src/java/org/apache/hadoop/hive/ql/parse/repl/load/message/AddUniqueConstraintHandler.java 81f1c5ab20
ql/src/java/org/apache/hadoop/hive/ql/parse/repl/load/message/DropConstraintHandler.java 5f9f879f6f
ql/src/java/org/apache/hadoop/hive/ql/plan/AlterTableDesc.java 8603521041
ql/src/java/org/apache/hadoop/hive/ql/plan/DDLWork.java 6cd84bb8ab
ql/src/java/org/apache/hadoop/hive/ql/plan/ShowColumnsDesc.java 7047f56275
ql/src/test/queries/clientpositive/allow_change_col_type_par.q aad63705f7
ql/src/test/queries/clientpositive/avro_alter_table_update_columns.q 279d05d2e3
ql/src/test/queries/clientpositive/check_constraint.q 202110259d
ql/src/test/queries/clientpositive/rename_column.q 96daf9d658
ql/src/test/queries/clientpositive/show_columns.q aa45bae9d5
ql/src/test/results/clientnegative/allow_change_col_type_par_neg.q.out 3f91e85a3a
ql/src/test/results/clientnegative/alter_partition_change_col_dup_col.q.out 542e85cb89
ql/src/test/results/clientnegative/alter_partition_change_col_nonexist.q.out bc5a6f980f
ql/src/test/results/clientnegative/alter_table_constraint_duplicate_pk.q.out acf65f2ff6
ql/src/test/results/clientnegative/alter_table_constraint_invalid_fk_col1.q.out 1617609ce2
ql/src/test/results/clientnegative/alter_table_constraint_invalid_fk_col2.q.out 47166ac6c2
ql/src/test/results/clientnegative/alter_table_constraint_invalid_fk_tbl1.q.out 49bc928ac1
ql/src/test/results/clientnegative/alter_table_constraint_invalid_fk_tbl2.q.out f5ac4ac54a
ql/src/test/results/clientnegative/alter_table_constraint_invalid_pk_col.q.out 71689f70ee
ql/src/test/results/clientnegative/alter_table_constraint_invalid_pk_tbl.q.out 792134cbe6
ql/src/test/results/clientnegative/alter_table_constraint_invalid_ref.q.out 9e98454f5b
ql/src/test/results/clientnegative/altern1.q.out ff3c670864
ql/src/test/results/clientnegative/avro_add_column_extschema.q.out a1b1b9ca6e
ql/src/test/results/clientnegative/column_rename1.q.out 01549c2665
ql/src/test/results/clientnegative/column_rename2.q.out 41c2219d10
ql/src/test/results/clientnegative/column_rename4.q.out d5729f127a
ql/src/test/results/clientnegative/disallow_incompatible_type_change_on1.q.out 8fe4c05872
ql/src/test/results/clientnegative/disallow_incompatible_type_change_on2.q.out 6291feb931
ql/src/test/results/clientnegative/drop_invalid_constraint1.q.out 2cb3996015
ql/src/test/results/clientnegative/drop_invalid_constraint2.q.out 04352b40d7
ql/src/test/results/clientnegative/drop_invalid_constraint3.q.out 03e4bd6097
ql/src/test/results/clientnegative/drop_invalid_constraint4.q.out 473dec7a4c
ql/src/test/results/clientnegative/hms_using_serde_alter_table_update_columns.q.out 202acd7e31
ql/src/test/results/clientnegative/orc_reorder_columns1.q.out c581f4e312
ql/src/test/results/clientnegative/orc_reorder_columns1_acid.q.out 8f7255c973
ql/src/test/results/clientnegative/orc_reorder_columns2.q.out 54dcdecf1c
ql/src/test/results/clientnegative/orc_reorder_columns2_acid.q.out 6cae15b81a
ql/src/test/results/clientnegative/orc_replace_columns1.q.out 13f3f1448b
ql/src/test/results/clientnegative/orc_replace_columns1_acid.q.out 46caec214f
ql/src/test/results/clientnegative/orc_replace_columns2.q.out 2316bbbf35
ql/src/test/results/clientnegative/orc_replace_columns2_acid.q.out e01b7b9edb
ql/src/test/results/clientnegative/orc_replace_columns3.q.out a7b3b72ced
ql/src/test/results/clientnegative/orc_replace_columns3_acid.q.out b82ad57f6e
ql/src/test/results/clientnegative/orc_type_promotion1.q.out f45283664a
ql/src/test/results/clientnegative/orc_type_promotion1_acid.q.out 49800a96df
ql/src/test/results/clientnegative/orc_type_promotion2.q.out 740ee1e850
ql/src/test/results/clientnegative/orc_type_promotion2_acid.q.out 28c789a20a
ql/src/test/results/clientnegative/orc_type_promotion3.q.out 4f97e3118e
ql/src/test/results/clientnegative/orc_type_promotion3_acid.q.out a214985f94
ql/src/test/results/clientnegative/parquet_alter_part_table_drop_columns.q.out d22d9c8763
ql/src/test/results/clientpositive/allow_change_col_type_par.q.out e9570370ea
ql/src/test/results/clientpositive/avro_alter_table_update_columns.q.out 7f74f6c141
ql/src/test/results/clientpositive/input3.q.out 0ac95780cb
ql/src/test/results/clientpositive/llap/check_constraint.q.out 297d8928f8
ql/src/test/results/clientpositive/rename_column.q.out 43abc7f72a
ql/src/test/results/clientpositive/show_columns.q.out 80d69a7276
Diff: https://reviews.apache.org/r/70646/diff/2/
Changes: https://reviews.apache.org/r/70646/diff/1-2/
Testing
-------
Added new unit tests + all the old unit tests are still running fine.
Thanks,
Miklos Gergely