----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72139/#review219607 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/ddl/DDLSemanticAnalyzerFactory.java Line 52 (original), 52 (patched) <https://reviews.apache.org/r/72139/#comment307794> I don't really like the 2 type/types with defaults annotation. I think we can remove the default-s and the 2 methods for the same thing ``` @Retention(RetentionPolicy.RUNTIME) public @interface DDLType { int[] types(); } // and then the following 2 is valid: @DDLType(types = 2) class a { } @DDLType(types = { 2, 3 }) class b { } ``` another alternative would be to introduce an annotation to encapsulate multiple DDLTypes https://stackoverflow.com/questions/1554112/multiple-annotations-of-the-same-type-on-one-element ql/src/java/org/apache/hadoop/hive/ql/ddl/DDLSemanticAnalyzerFactory.java Line 79 (original), 81 (patched) <https://reviews.apache.org/r/72139/#comment307793> add duplicate detection; if by mistake somehow a re-registeration may occur that could lead to odd behaviour - Zoltan Haindrich On Feb. 16, 2020, 7:22 p.m., Miklos Gergely wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72139/ > ----------------------------------------------------------- > > (Updated Feb. 16, 2020, 7:22 p.m.) > > > Review request for hive and Zoltan Haindrich. > > > Bugs: HIVE-22895 > https://issues.apache.org/jira/browse/HIVE-22895 > > > Repository: hive-git > > > Description > ------- > > DDLSemanticAnalyzer is a huge class, more than 4000 lines long. The goal is > to refactor it 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 analyzers > have a package for each operation, containing an analyzer, a description, and > an operation, so the amount of classes under a package is more manageable > Step #14: extract the table storage related analyzers from > DDLSemanticAnalyzer, and move them under the new package. > > > Diffs > ----- > > > hcatalog/core/src/main/java/org/apache/hive/hcatalog/cli/SemanticAnalysis/HCatSemanticAnalyzer.java > 8277d34731 > ql/src/java/org/apache/hadoop/hive/ql/ddl/DDLSemanticAnalyzerFactory.java > 119b555d58 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/AbstractAlterTableAnalyzer.java > 81800fe000 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableArchiveDesc.java > 9dd6c8ec7d > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableArchiveOperation.java > 248fe0f5a3 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableArchiveUtils.java > c285405522 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableClusteredByDesc.java > a9a4724ea7 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableClusteredByOperation.java > c232e66760 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableCompactDesc.java > 3a512ba6e4 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableCompactOperation.java > 8e576fa446 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableConcatenateDesc.java > 5f5bbe4a0d > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableConcatenateOperation.java > ea21e026bf > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableIntoBucketsDesc.java > c8d1a599db > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableIntoBucketsOperation.java > f7d224bddd > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableNotClusteredDesc.java > 37005f658d > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableNotClusteredOperation.java > 9b4fb32881 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableNotSkewedDesc.java > 016c18c60e > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableNotSkewedOperation.java > cb4632d309 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableNotSortedDesc.java > 30614f2dce > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableNotSortedOperation.java > 3d3996d0d3 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableSetFileFormatDesc.java > 78ac94b3b5 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableSetFileFormatOperation.java > 5b7c5acbe4 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableSetLocationDesc.java > d79a8e4751 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableSetLocationOperation.java > 509d5770d3 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableSetSerdeDesc.java > 6038cd7b34 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableSetSerdeOperation.java > a447b3186e > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableSetSerdePropsDesc.java > fdbdcf5573 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableSetSerdePropsOperation.java > 58654ccda9 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableSetSkewedLocationDesc.java > 4cae3b0251 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableSetSkewedLocationOperation.java > 3d9f9cebad > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableSkewedByDesc.java > 656aaa6ea9 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableSkewedByOperation.java > 1275565a82 > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableUnarchiveDesc.java > 06889ae9ac > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/AlterTableUnarchiveOperation.java > 39416ede9d > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/archive/AbstractAlterTableArchiveAnalyzer.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/archive/AlterTableArchiveAnalyzer.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/archive/AlterTableUnarchiveAnalyzer.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/cluster/AlterTableClusterSortAnalyzer.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/cluster/AlterTableIntoBucketsAnalyzer.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/compact/AlterTableCompactAnalyzer.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/concatenate/AlterTableConcatenateAnalyzer.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/serde/AlterTableSetSerdeAnalyzer.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/serde/AlterTableSetSerdePropsAnalyzer.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/set/fileformat/AlterTableSetFileFormatAnalyzer.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/set/location/AlterTableSetLocationAnalyzer.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/skewed/AlterTableSetSkewedLocationAnalyzer.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/skewed/AlterTableSkewedByAnalyzer.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/skewed/SkewedTableUtils.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java > f7ac6d3bfa > ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java > eee81a39e0 > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java > 33d3beba46 > ql/src/test/org/apache/hadoop/hive/ql/parse/TestQBCompact.java 0ec4605c31 > ql/src/test/queries/clientnegative/compact_illegal_type.q PRE-CREATION > ql/src/test/results/clientnegative/compact_illegal_type.q.out PRE-CREATION > ql/src/test/results/clientnegative/merge_negative_4.q.out d9eb2219bd > > > Diff: https://reviews.apache.org/r/72139/diff/1/ > > > Testing > ------- > > Tests are still passing. > > > Thanks, > > Miklos Gergely > >
