hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gabor Szadovszky <gabor.szadovs...@cloudera.com>
Subject Re: Review Request 51046: Support explain analyze in Hive
Date Fri, 26 Aug 2016 23:44:01 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message