Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Hive >> mail # dev >> Review Request 24289: MetadataUpdater: provide a mechanism to edit the statistics of a column in a table (or a partition of a table)


Copy link to this message
-
Re: Review Request 24289: MetadataUpdater: provide a mechanism to edit the statistics of a column in a table (or a partition of a table)

This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24289/#review49661
This is as far as I've gotten today. I'll try to continue tomorrow.
ql/src/java/org/apache/hadoop/hive/ql/exec/ColumnStatsUpdateTask.java
<https://reviews.apache.org/r/24289/#comment86941>

    redundant

ql/src/java/org/apache/hadoop/hive/ql/exec/ColumnStatsUpdateTask.java
<https://reviews.apache.org/r/24289/#comment86942>

    should be private static final transient and no need to be wrapped

ql/src/java/org/apache/hadoop/hive/ql/exec/ColumnStatsUpdateTask.java
<https://reviews.apache.org/r/24289/#comment86943>

    redundant constructor

ql/src/java/org/apache/hadoop/hive/ql/exec/ColumnStatsUpdateTask.java
<https://reviews.apache.org/r/24289/#comment86945>

    Map instead of HashMap

ql/src/java/org/apache/hadoop/hive/ql/exec/TaskFactory.java
<https://reviews.apache.org/r/24289/#comment86940>

    long line (hive uses a maximum of 100 chars)

ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java
<https://reviews.apache.org/r/24289/#comment86939>

    unrelated

ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java
<https://reviews.apache.org/r/24289/#comment86936>

    missing space

ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java
<https://reviews.apache.org/r/24289/#comment86928>

    Indentation is wrong in this whole method (and the next one), should be two spaces

ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java
<https://reviews.apache.org/r/24289/#comment86929>

    Exception never thrown (same for next class)

ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java
<https://reviews.apache.org/r/24289/#comment86931>

    properly handle and log. Like this it'll throw a NPE later on tbl.getCols() if this goes wrong.
    
    Alternatively maybe just ignore the exception. It's an unchecked one and there's not much you can do about it anyway

ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java
<https://reviews.apache.org/r/24289/#comment86934>

    You're not checking if this actually results in anything. Will result in another NPE later on.

ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java
<https://reviews.apache.org/r/24289/#comment86933>

    no need to explicitly create an array
    
    Arrays.asList(colName) etc. works as well.

ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java
<https://reviews.apache.org/r/24289/#comment86938>

    There's a lot of copy & paste in these two methods. They only differ in the partition stuff which should be fairly easy to stuff into one method. Haven't checked though...either way the comments from the previous method apply here too

ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java
<https://reviews.apache.org/r/24289/#comment86935>

    can be Map instead of HashMap

ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java
<https://reviews.apache.org/r/24289/#comment86919>

    tab instead of spaces
    
    braces around the return statement
    
    I also don't fully understand this part but from what I understand you circumvent authentication checking here?

ql/src/java/org/apache/hadoop/hive/ql/plan/ColumnStatsUpdateWork.java
<https://reviews.apache.org/r/24289/#comment86908>

    Remove or expand this comment. Leaving it in brings no additional benefit and removing it will at least flag the class as undocumented.

ql/src/java/org/apache/hadoop/hive/ql/plan/ColumnStatsUpdateWork.java
<https://reviews.apache.org/r/24289/#comment86909>

    should be Map instead of HashMap

ql/src/java/org/apache/hadoop/hive/ql/plan/ColumnStatsUpdateWork.java
<https://reviews.apache.org/r/24289/#comment86911>

    Constructor is not used anywhere and can be removed

ql/src/java/org/apache/hadoop/hive/ql/plan/ColumnStatsUpdateWork.java
<https://reviews.apache.org/r/24289/#comment86910>

    should be Map instead of HashMap

ql/src/java/org/apache/hadoop/hive/ql/plan/ColumnStatsUpdateWork.java
<https://reviews.apache.org/r/24289/#comment86915>

    No need to call setters in the constructor

ql/src/java/org/apache/hadoop/hive/ql/plan/ColumnStatsUpdateWork.java
<https://reviews.apache.org/r/24289/#comment86914>

    Are any of the setters actually needed? If not you could remove them and make the fields private. They don't seem to be used anywhere...

ql/src/java/org/apache/hadoop/hive/ql/plan/ColumnStatsUpdateWork.java
<https://reviews.apache.org/r/24289/#comment86912>

    should be Map instead of HashMap

ql/src/java/org/apache/hadoop/hive/ql/plan/ColumnStatsUpdateWork.java
<https://reviews.apache.org/r/24289/#comment86913>

    should be Map instead of HashMap
- Lars Francke
On Aug. 5, 2014, 6:40 p.m., pengcheng xiong wrote: