----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51046/#review147033 -----------------------------------------------------------
Some nits and minor findings. LGTM, otherwise. Thanks for the patch. ql/src/java/org/apache/hadoop/hive/ql/exec/Operator.java (line 1448) <https://reviews.apache.org/r/51046/#comment214009> The comment "just return, ..." seems to contradict with this throw clause. ql/src/java/org/apache/hadoop/hive/ql/exec/ReduceSinkOperator.java (line 64) <https://reviews.apache.org/r/51046/#comment214011> nit: seems to be some formatting anomaly (too many spaces before '{') ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java (line 300) <https://reviews.apache.org/r/51046/#comment214015> As it is public static wouldn't it have a better place in a utility class (e.g. StatsUtils)? ql/src/java/org/apache/hadoop/hive/ql/hooks/ATSHook.java (line 132) <https://reviews.apache.org/r/51046/#comment214016> nit: Seems to break the previous commenting structure by adding the two parameters in one line. Might be better to separate them to two lines and add some comment for config (if it make sense). ql/src/java/org/apache/hadoop/hive/ql/optimizer/Optimizer.java (line 231) <https://reviews.apache.org/r/51046/#comment214017> nit: remove trailing whitespace ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/AnnotateRunTimeStatsOptimizer.java (line 57) <https://reviews.apache.org/r/51046/#comment214018> I would recommend having the Logger instance private as it is created specifically for the actual class. (BTW: .getName() is not required as LoggerFactory has a getLogger(Class<?>) as well) ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/PhysicalOptimizer.java (line 98) <https://reviews.apache.org/r/51046/#comment214021> nit: Trailing whitespaces should be removed. ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainConfiguration.java (line 37) <https://reviews.apache.org/r/51046/#comment214027> I would suggest having this field private instead of package private. (It already has public getter and setter anyway.) ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainConfiguration.java (line 39) <https://reviews.apache.org/r/51046/#comment214006> By java coding conventions it should be AnalyzeState instead. ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainConfiguration.java (line 43) <https://reviews.apache.org/r/51046/#comment214025> I would suggest having this private instead package private. (It already has public getter and setter anyway.) ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainSQRewriteSemanticAnalyzer.java (line 40) <https://reviews.apache.org/r/51046/#comment214024> nit: Trailing whitespaces should be removed. ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainSemanticAnalyzer.java (line 151) <https://reviews.apache.org/r/51046/#comment214028> nit: Trailing whitespaces should be removed. ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java (line 504) <https://reviews.apache.org/r/51046/#comment214029> nit: Trailing whitespaces should be removed. - Gabor Szadovszky On Aug. 26, 2016, 8:28 p.m., pengcheng xiong wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51046/ > ----------------------------------------------------------- > > (Updated Aug. 26, 2016, 8:28 p.m.) > > > Review request for hive and Ashutosh Chauhan. > > > Repository: hive-git > > > Description > ------- > > HIVE-14362 > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/common/StatsSetupConst.java 559fffc > itests/src/test/resources/testconfiguration.properties dfde5e2 > ql/src/java/org/apache/hadoop/hive/ql/Context.java 3785b1e > ql/src/java/org/apache/hadoop/hive/ql/Driver.java 183ed82 > ql/src/java/org/apache/hadoop/hive/ql/exec/ColumnStatsTask.java a183b9b > ql/src/java/org/apache/hadoop/hive/ql/exec/CommonJoinOperator.java 43231af > ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java a59b781 > ql/src/java/org/apache/hadoop/hive/ql/exec/ExplainTask.java ad48091 > ql/src/java/org/apache/hadoop/hive/ql/exec/FileSinkOperator.java b0c3d3f > ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java 47b5793 > ql/src/java/org/apache/hadoop/hive/ql/exec/JoinOperator.java 08cc4b4 > ql/src/java/org/apache/hadoop/hive/ql/exec/LimitOperator.java 9676d70 > ql/src/java/org/apache/hadoop/hive/ql/exec/ListSinkOperator.java 9bf363c > ql/src/java/org/apache/hadoop/hive/ql/exec/MoveTask.java 546919b > ql/src/java/org/apache/hadoop/hive/ql/exec/Operator.java eaf4792 > ql/src/java/org/apache/hadoop/hive/ql/exec/ReduceSinkOperator.java ba71a1e > ql/src/java/org/apache/hadoop/hive/ql/exec/SerializationUtilities.java > 42c1003 > ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java e1f7bd9 > ql/src/java/org/apache/hadoop/hive/ql/exec/TableScanOperator.java 6afe957 > ql/src/java/org/apache/hadoop/hive/ql/exec/UDTFOperator.java a75b52a > ql/src/java/org/apache/hadoop/hive/ql/hooks/ATSHook.java 742edc8 > ql/src/java/org/apache/hadoop/hive/ql/optimizer/Optimizer.java 5ee54b9 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/AnnotateRunTimeStatsOptimizer.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/PhysicalOptimizer.java > 49706b1 > > ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsAutoGatherContext.java > 15a47dc > > ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java > d3aef41 > ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainConfiguration.java > PRE-CREATION > > ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainSQRewriteSemanticAnalyzer.java > 8d7fd92 > ql/src/java/org/apache/hadoop/hive/ql/parse/ExplainSemanticAnalyzer.java > 75753b0 > ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 6715dbf > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g c411f5e > ql/src/java/org/apache/hadoop/hive/ql/parse/MapReduceCompiler.java 5b08ed2 > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 66589fe > ql/src/java/org/apache/hadoop/hive/ql/parse/SubQueryDiagnostic.java 57f9432 > ql/src/java/org/apache/hadoop/hive/ql/parse/TaskCompiler.java 114fa2f > ql/src/java/org/apache/hadoop/hive/ql/parse/TezCompiler.java 66a8322 > > ql/src/java/org/apache/hadoop/hive/ql/parse/UpdateDeleteSemanticAnalyzer.java > 33fbffe > ql/src/java/org/apache/hadoop/hive/ql/parse/spark/SparkCompiler.java > 08278de > ql/src/java/org/apache/hadoop/hive/ql/plan/AbstractOperatorDesc.java > adec5c7 > ql/src/java/org/apache/hadoop/hive/ql/plan/ExplainWork.java a213c83 > ql/src/java/org/apache/hadoop/hive/ql/plan/FileSinkDesc.java ce0e0a8 > ql/src/java/org/apache/hadoop/hive/ql/plan/MergeJoinWork.java a5527dc > ql/src/java/org/apache/hadoop/hive/ql/plan/OperatorDesc.java 16be499 > ql/src/java/org/apache/hadoop/hive/ql/plan/Statistics.java 029043f > ql/src/test/org/apache/hadoop/hive/ql/exec/TestExplainTask.java 990d80c > > ql/src/test/org/apache/hadoop/hive/ql/parse/TestUpdateDeleteSemanticAnalyzer.java > ae1747d > ql/src/test/queries/clientpositive/explainanalyze_0.q PRE-CREATION > ql/src/test/queries/clientpositive/explainanalyze_1.q PRE-CREATION > ql/src/test/queries/clientpositive/explainanalyze_2.q PRE-CREATION > ql/src/test/queries/clientpositive/explainanalyze_3.q PRE-CREATION > ql/src/test/queries/clientpositive/explainanalyze_4.q PRE-CREATION > ql/src/test/queries/clientpositive/explainanalyze_5.q PRE-CREATION > ql/src/test/results/clientpositive/columnstats_partlvl.q.out f6f2bfa > ql/src/test/results/clientpositive/columnstats_partlvl_dp.q.out 21089e1 > ql/src/test/results/clientpositive/columnstats_quoting.q.out 288e61b > ql/src/test/results/clientpositive/columnstats_tbllvl.q.out 8d280c1 > ql/src/test/results/clientpositive/compute_stats_date.q.out eaf4bbe > ql/src/test/results/clientpositive/constant_prop_2.q.out c1de559 > ql/src/test/results/clientpositive/display_colstats_tbllvl.q.out 42aeb6f > ql/src/test/results/clientpositive/dynpart_sort_optimization_acid.q.out > de08dc5 > ql/src/test/results/clientpositive/exec_parallel_column_stats.q.out 4fcde91 > ql/src/test/results/clientpositive/temp_table_display_colstats_tbllvl.q.out > e229dba > ql/src/test/results/clientpositive/tez/explainanalyze_0.q.out PRE-CREATION > ql/src/test/results/clientpositive/tez/explainanalyze_1.q.out PRE-CREATION > ql/src/test/results/clientpositive/tez/explainanalyze_2.q.out PRE-CREATION > ql/src/test/results/clientpositive/tez/explainanalyze_3.q.out PRE-CREATION > ql/src/test/results/clientpositive/tez/explainanalyze_4.q.out PRE-CREATION > ql/src/test/results/clientpositive/tez/explainanalyze_5.q.out PRE-CREATION > > Diff: https://reviews.apache.org/r/51046/diff/ > > > Testing > ------- > > > Thanks, > > pengcheng xiong > >
