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

Switch to Threaded View
Hive >> mail # dev >> Review Request 15561: HIVE-5771 Constant propagation optimizer for Hive


Copy link to this message
-
Re: Review Request 15561: HIVE-5771 Constant propagation optimizer for Hive

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15561/#review28999
-----------------------------------------------------------

http://svn.apache.org/repos/asf/hive/trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
<https://reviews.apache.org/r/15561/#comment56043>

    Please eliminate trailing white space.
    
    I recommend changing
    HIVEOPTCONSTANTPROG
    
    to
    HIVEOPTCONSTANTPROPAGATION

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagate.java
<https://reviews.apache.org/r/15561/#comment56044>

    Please remove trailing white space in all your new code. You should be able to set your editor to help do this (Eclipse can do this).

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagate.java
<https://reviews.apache.org/r/15561/#comment56046>

    Please clarify this comment (3). I'm not sure what it means.

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagate.java
<https://reviews.apache.org/r/15561/#comment56047>

    Coding style guidelines say to but a blank line before all comments. Please check all your comments for this.

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagate.java
<https://reviews.apache.org/r/15561/#comment56048>

    What is a topop node? Please elaborate.

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcCtx.java
<https://reviews.apache.org/r/15561/#comment56050>

    Please elaborate a little bit on what this context is.

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcCtx.java
<https://reviews.apache.org/r/15561/#comment56049>

    trailing white space

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcCtx.java
<https://reviews.apache.org/r/15561/#comment56051>

    Please add a descriptive comment for this method.
    
    Also, do you have a design specification or document? This is complex enough that it is important to have that.

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcCtx.java
<https://reviews.apache.org/r/15561/#comment56052>

    Please add a descriptive comment before this method

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcFactory.java
<https://reviews.apache.org/r/15561/#comment56053>

    Please add a descriptive comment before this method

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcFactory.java
<https://reviews.apache.org/r/15561/#comment56054>

    Please add a descriptive comment before this method

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcFactory.java
<https://reviews.apache.org/r/15561/#comment56055>

    Funtion -> function
    foding -> folding

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcFactory.java
<https://reviews.apache.org/r/15561/#comment56056>

    Please add a descriptive comment before this method

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcFactory.java
<https://reviews.apache.org/r/15561/#comment56058>

    Need *to* convert type
    

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcFactory.java
<https://reviews.apache.org/r/15561/#comment56060>

    remove this commented assert?

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcFactory.java
<https://reviews.apache.org/r/15561/#comment56059>

    should say "never supposed to go here"
    
    If it is really an error to ever go here, why not throw an exception? That way the code will get debugged faster.
    
    If you are supposed to go here then remove the comment that says you're not supposed to go here.

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcFactory.java
<https://reviews.apache.org/r/15561/#comment56061>

    remove trailing blank

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcFactory.java
<https://reviews.apache.org/r/15561/#comment56062>

    Please add a descriptive comment before this method

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcFactory.java
<https://reviews.apache.org/r/15561/#comment56063>

    Please elaborate a little in the comment
    
    Probably should say "Constant Propagation" not "Constant Propagate"
    

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcFactory.java
<https://reviews.apache.org/r/15561/#comment56064>

    hold true -> holds true

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcFactory.java
<https://reviews.apache.org/r/15561/#comment56065>

    holds false

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcFactory.java
<https://reviews.apache.org/r/15561/#comment56066>

    Propagate on -> Propagation for

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConstantPropagateProcFactory.java
<https://reviews.apache.org/r/15561/#comment56067>

    trailing blank

http://svn.apache.org/repos/asf/h